diff --git a/scripts/architecture-baseline.json b/scripts/architecture-baseline.json index d6e9bfa..3d1514d 100644 --- a/scripts/architecture-baseline.json +++ b/scripts/architecture-baseline.json @@ -173,6 +173,7 @@ "clone 2c627b34911b src/codeintel/__tests__/call-graph.test.ts,src/codeintel/__tests__/hover.test.ts,src/codeintel/__tests__/import-graph.test.ts,src/codeintel/__tests__/outline.test.ts,src/codeintel/__tests__/rename-symbol.test.ts", "clone 2c83ed0d1788 src/engine/__tests__/condense-async.test.ts,src/engine/__tests__/condense.test.ts,src/engine/__tests__/events.test.ts", "clone 2c9f8493e38e src/llm/muse.ts,src/llm/openrouter.ts", + "clone 2cb1493c835d src/orchestrator/__tests__/main-tree-tripwire.test.ts,src/orchestrator/__tests__/review-gates.test.ts", "clone 2e4f5c004508 src/init/__tests__/cli.test.ts,src/orchestrator/__tests__/cost-history-cli.test.ts", "clone 2eefefd492bd src/checkpoints/__tests__/cli.test.ts,src/codemap/__tests__/cli.test.ts,src/codemap/__tests__/units.test.ts,src/dashboard/__tests__/cli.test.ts,src/dashboard/__tests__/page-route.test.ts,src/dashboard/__tests__/self-improvement-metrics.test.ts,src/dashboard/__tests__/sigint-shutdown.test.ts,src/github/__tests__/installations.test.ts,src/orchestrator/__tests__/analyze-cli.test.ts,src/orchestrator/__tests__/cost-estimate.test.ts,src/orchestrator/__tests__/log-analysis.test.ts,src/orchestrator/__tests__/spawn-state-reset.test.ts,src/orchestrator/__tests__/status.test.ts,src/run-tests-script.test.ts,src/tools/__tests__/set-indentation.test.ts", "clone 2ef48d0cbf0f src/engine/__tests__/new-task.test.ts,src/engine/__tests__/switch-mode.test.ts", @@ -254,7 +255,7 @@ "clone 44f4c5f6148f src/cloud/__tests__/openshell-provider.test.ts", "clone 45181f328e82 src/orchestrator/task-files.ts", "clone 45edfde1d33f src/orchestrator/task-files.ts", - "clone 464303e3e594 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/review-gates.test.ts", + "clone 464303e3e594 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/main-tree-tripwire.test.ts,src/orchestrator/__tests__/review-gates.test.ts", "clone 465b90f368f7 src/dashboard/__tests__/server.test.ts,src/dashboard/__tests__/session-launch.test.ts", "clone 4666d04d2662 src/llm/muse.ts,src/llm/openrouter.ts", "clone 46ffdeff854a src/orchestrator/resume.ts", @@ -384,6 +385,7 @@ "clone 68612079ddc5 src/llm/preflight.ts", "clone 68654f999273 src/engine/__tests__/stack-rules-content.test.ts,src/stack-rules-content.test.ts", "clone 687e46486dc5 src/engine/__tests__/stack-rules-content.test.ts,src/engine/__tests__/stacks.test.ts", + "clone 6893410ec1b1 src/orchestrator/__tests__/main-tree-tripwire.test.ts,src/orchestrator/__tests__/review-gates.test.ts", "clone 68b1356447b2 src/orchestrator/cli.ts", "clone 68ceda9cfcd3 src/codesearch/files.ts,src/engine/stacks.ts", "clone 694e054dcbfb src/codesearch/files.ts,src/engine/stacks.ts", @@ -764,7 +766,7 @@ "clone c7d2cfba43b6 src/engine/__tests__/new-task.test.ts,src/engine/__tests__/switch-mode.test.ts", "clone c9648e69e45e src/orchestrator/__tests__/cli.test.ts", "clone c9726dcfaf3b src/engine/local-explore.ts,src/llm/ollama.ts", - "clone c9fe2d9273f4 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/review-gates.test.ts", + "clone c9fe2d9273f4 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/main-tree-tripwire.test.ts,src/orchestrator/__tests__/review-gates.test.ts", "clone ca31b6bb0f50 src/budget/__tests__/concurrency.test.ts,src/watcher/__tests__/watch.test.ts", "clone cb208607f464 src/dashboard/server.ts", "clone cb54ce2ca134 src/orchestrator/__tests__/cli.test.ts,src/orchestrator/__tests__/resume.test.ts", @@ -826,7 +828,7 @@ "clone d8dfa6d1a846 src/orchestrator/cli.ts", "clone d8e97665e9ec src/checkpoints/__tests__/cli.test.ts,src/codemap/__tests__/cli.test.ts,src/dashboard/__tests__/cli.test.ts", "clone d9003b766f88 src/dashboard/__tests__/server.test.ts,src/dashboard/__tests__/session-launch.test.ts", - "clone d95677f0fc38 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/review-gates.test.ts", + "clone d95677f0fc38 src/codeintel/__tests__/implementations.test.ts,src/orchestrator/__tests__/main-tree-tripwire.test.ts,src/orchestrator/__tests__/review-gates.test.ts", "clone d96d4c124839 src/dashboard/trend-cli.ts,src/orchestrator/cleanup.ts", "clone d9ab3b61fdbc src/llm/preflight.ts", "clone d9cbef8f79d1 src/engine/local-explore.ts,src/llm/ollama.ts", @@ -1252,7 +1254,6 @@ "src/permissions/commands.ts complexity 88bfec36eee2", "src/permissions/commands.ts complexity a59b0df8679a", "src/permissions/commands.ts complexity bcaa684da117", - "src/permissions/commands.ts complexity c3e30364b4e2", "src/permissions/commands.ts nesting bcaa684da117", "src/permissions/commands.ts params 300fe7554f69", "src/permissions/config.ts complexity addac3b5a99c", diff --git a/scripts/check-main-tree-clean.sh b/scripts/check-main-tree-clean.sh new file mode 100755 index 0000000..fe47f31 --- /dev/null +++ b/scripts/check-main-tree-clean.sh @@ -0,0 +1,123 @@ +#!/usr/bin/env bash +# check-main-tree-clean.sh — issue #136 Part B tripwire. +# +# A worker MUST write only inside its own worktree. Part A (the +# execute_command confinement gate) blocks relative-climb writes for the +# builtin executor, but the harness cannot prove a negative for every +# provider and every bypass (sudo/ssh/xargs -exec, guessed absolute +# paths, ...). This script is the backstop: run-worker.sh / run-qa.sh +# snapshot the MAIN checkout's git status before the worker starts and +# diff it right after the worker exits. Anything NEW in main — outside +# the orchestrator's own dirs — means the worker wrote outside its +# worktree. The finding is recorded in a marker file the orchestrator +# surfaces (watch.ts / reconcileGroups persist it as `main_tree_dirty`) +# and echoed loudly to stderr: the wrapper redirects stderr into +# harness.log/qa.log, while stdout stays clean for PID capture. +# +# Usage: check-main-tree-clean.sh +# Always exits 0: tripwire infra must never fail (or pass) a worker run. +# +# Exclusions (orchestrator-owned; churn mid-run in multi-group rounds): +# .worktrees/ other groups' worktrees + .orchestrator-state.json +# plans/parallel-tasks/ task/rework/continuation files for other groups +# Documented blind spot: a worker escaping INTO those two dirs evades +# this tripwire. plans/issues/* and everything else is covered. +# Concurrent operator edits to main during the run window also trip the +# wire — the marker is evidence for a human/reviewer, NOT an auto-fail. +set -u -o pipefail + +# All diagnostics go to stderr, never stdout: run-worker.sh prints the +# wrapper PID on stdout (a contract stdout-capturing callers and tests +# rely on), and the wrapper redirects stderr into harness.log/qa.log +# anyway, so stderr stays loud in exactly the right place. +exec >&2 + +ROLE="${1:?usage: check-main-tree-clean.sh }" +MODE="${2:?usage: check-main-tree-clean.sh }" +WT_PATH="${3:?usage: check-main-tree-clean.sh }" + +case "$ROLE" in + worker) STEM=".harness" ;; + qa) STEM=".qa" ;; + *) echo "TRIPWIRE-SKIPPED: bad role '$ROLE'" ; exit 0 ;; +esac +case "$MODE" in + snapshot | check) ;; + *) echo "TRIPWIRE-SKIPPED: bad mode '$MODE'" ; exit 0 ;; +esac + +BEFORE="$WT_PATH/$STEM.main-tree-before" +MARKER="$WT_PATH/$STEM.main-tree-dirty" + +# The MAIN checkout is the common git dir's parent. Fail open with a +# loud warning when git metadata is unusable — never fail the worker. +COMMON="$(git -C "$WT_PATH" rev-parse --git-common-dir 2>/dev/null)" || { + echo "TRIPWIRE-SKIPPED: cannot resolve main checkout from '$WT_PATH'" + exit 0 +} +case "$COMMON" in + /*) MAIN_DIR="$COMMON" ;; + *) MAIN_DIR="$WT_PATH/$COMMON" ;; +esac +WT_REAL="$(cd "$WT_PATH" 2>/dev/null && pwd)" || { + echo "TRIPWIRE-SKIPPED: worktree '$WT_PATH' unreadable" + exit 0 +} +MAIN_REAL="$(cd "$MAIN_DIR/.." 2>/dev/null && pwd)" || { + echo "TRIPWIRE-SKIPPED: main checkout under '$MAIN_DIR' unreadable" + exit 0 +} +if [ "$WT_REAL" = "$MAIN_REAL" ]; then + echo "TRIPWIRE-SKIPPED: worker running directly on main checkout '$MAIN_REAL' — confinement by review only" + exit 0 +fi + +# One porcelain snapshot of main, minus orchestrator-owned dirs. Rename +# entries (`R old -> new`) match on the NEW path too. +snapshot_status() { + git -C "$MAIN_REAL" status --porcelain -uall 2>/dev/null | awk ' + { + line = $0; sub(/^.{3}/, "", line); p = line + i = index(line, " -> ") + if (i > 0) p = substr(line, i + 4) + if (index(p, ".worktrees/") == 1) next + if (index(p, "plans/parallel-tasks/") == 1) next + print + } + ' +} + +if [ "$MODE" = "snapshot" ]; then + TMP="$BEFORE.tmp.$$" + if snapshot_status >"$TMP"; then + mv "$TMP" "$BEFORE" + else + rm -f "$TMP" + echo "TRIPWIRE-SKIPPED: cannot snapshot main checkout '$MAIN_REAL'" + fi + exit 0 +fi + +AFTER="$(snapshot_status)" || { + echo "TRIPWIRE-SKIPPED: cannot re-read main checkout '$MAIN_REAL'" + exit 0 +} +if [ -f "$BEFORE" ]; then + BEFORE_LINES="$(cat "$BEFORE")" +else + echo "TRIPWIRE: no baseline snapshot for '$WT_PATH' — treating all main-checkout dirt as new" + BEFORE_LINES="" +fi +NEW="$(comm -13 <(printf '%s\n' "$BEFORE_LINES" | sort) <(printf '%s\n' "$AFTER" | sort) | grep . || true)" +if [ -z "$NEW" ]; then + rm -f "$MARKER" + exit 0 +fi +printf '%s\n' "$NEW" >"$MARKER" +COUNT="$(printf '%s\n' "$NEW" | wc -l | tr -d ' ')" +WORD="entries" +[ "$COUNT" = "1" ] && WORD="entry" +echo "MAIN-TREE-DIRTY: worker '$WT_PATH' left $COUNT new $WORD in main checkout '$MAIN_REAL':" +printf '%s\n' "$NEW" | sed 's/^/MAIN-TREE-DIRTY: /' +echo "MAIN-TREE-DIRTY: marker: $MARKER (window between snapshot and worker exit; concurrent operator edits also match)" +exit 0 diff --git a/scripts/run-qa.sh b/scripts/run-qa.sh index acbbec8..6acfa33 100755 --- a/scripts/run-qa.sh +++ b/scripts/run-qa.sh @@ -147,11 +147,16 @@ if [ -n "${HEADLESSCODE_MAX_DURATION_MS:-}" ]; then BUDGET_ARG="$BUDGET_ARG --max-duration-ms '$HEADLESSCODE_MAX_DURATION_MS'" fi +# Issue #136 Part B: after QA exits, diff the MAIN checkout against the +# pre-launch snapshot — same tripwire as run-worker.sh (qa marker stem). +TRIPWIRE_CHECK="bash '$HARNESS_ROOT/scripts/check-main-tree-clean.sh' qa check '$WT_PATH' || echo 'TRIPWIRE-FAILED: main-tree check did not run — confinement unverified for this run'" + WRAPPER=" cd '$HARNESS_ROOT' $CLI --task-file '$TASK_FILE' --workspace '$WT_PATH' --mode '$MODE' $MODEL_ARG $LOG_FILE_ARG $BUDGET_ARG CODE=\$? printf '%s' \"\$CODE\" > '$WT_PATH/.qa.exit' +$TRIPWIRE_CHECK mkdir '$WT_PATH/.qa.done' 2>/dev/null exit \$CODE " @@ -159,6 +164,8 @@ exit \$CODE # ── Launch detached + record pid + pgid ───────────────────────────────────── rm -f "$WT_PATH/.qa.exit" rm -rf "$WT_PATH/.qa.done" +# Issue #136 Part B: snapshot the MAIN checkout BEFORE QA starts. +bash "$HARNESS_ROOT/scripts/check-main-tree-clean.sh" qa snapshot "$WT_PATH" || echo "run-qa: warning: main-tree tripwire snapshot failed" >&2 # Issue #20 (same fix as run-worker.sh): run the QA wrapper in its OWN process # group via setsid so a stop command can kill the whole tree, and record the diff --git a/scripts/run-worker.sh b/scripts/run-worker.sh index 20b2fc3..9072bb5 100755 --- a/scripts/run-worker.sh +++ b/scripts/run-worker.sh @@ -312,11 +312,18 @@ if [ -n "${HEADLESSCODE_WINDOW_SIZE:-}" ]; then BUDGET_ARG="$BUDGET_ARG --window-size '$HEADLESSCODE_WINDOW_SIZE'" fi +# Issue #136 Part B: after the worker exits, diff the MAIN checkout against +# the pre-launch snapshot (taken below) — a worker that wrote outside its +# worktree trips the wire (marker file + loud harness.log lines). Runs +# before mkdir .done, so a present .done marker implies the check ran. +TRIPWIRE_CHECK="bash '$HARNESS_ROOT/scripts/check-main-tree-clean.sh' worker check '$WT_PATH' || echo 'TRIPWIRE-FAILED: main-tree check did not run — confinement unverified for this run'" + WRAPPER=" cd '$HARNESS_ROOT' $CLI $TASK_FLAG '$TASK_PATH' --workspace '$WT_PATH' --mode '$MODE' $MODEL_ARG $LOG_FILE_ARG $MEMORY_DIR_ARG $BUDGET_ARG CODE=\$? printf '%s' \"\$CODE\" > '$WT_PATH/.harness.exit' +$TRIPWIRE_CHECK mkdir '$WT_PATH/.harness.done' 2>/dev/null exit \$CODE " @@ -332,6 +339,7 @@ if [ "$EXECUTOR" = "muse" ]; then PROMPT_CLEANUP="$MUSE_PROMPT_FILE" fi MAX_MODEL_STEPS_VALUE="${MAX_ITERATIONS:-${HEADLESSCODE_MAX_ITERATIONS:-}}" + TRIPWIRE_CHECK_Q="bash $(printf '%q' "$HARNESS_ROOT/scripts/check-main-tree-clean.sh") worker check $(printf '%q' "$WT_PATH") || echo $(printf '%q' "TRIPWIRE-FAILED: main-tree check did not run — confinement unverified for this run")" WRAPPER="cd $(printf '%q' "$HARNESS_ROOT") bash $(printf '%q' "$HARNESS_ROOT/scripts/run-muse-worker.sh") \ $(printf '%q' "$WT_PATH") $(printf '%q' "$MUSE_PROMPT_FILE") \ @@ -339,6 +347,7 @@ bash $(printf '%q' "$HARNESS_ROOT/scripts/run-muse-worker.sh") \ CODE=\$? if [ -n $(printf '%q' "$PROMPT_CLEANUP") ]; then rm -f -- $(printf '%q' "$PROMPT_CLEANUP"); fi printf '%s' \"\$CODE\" > $(printf '%q' "$WT_PATH/.harness.exit") +$TRIPWIRE_CHECK_Q mkdir $(printf '%q' "$WT_PATH/.harness.done") 2>/dev/null exit \$CODE " @@ -369,6 +378,7 @@ cd '$HARNESS_ROOT' $CLI${SANDBOX_ARGS_QUOTED} CODE=\$? printf '%s' \"\$CODE\" > '$WT_PATH/.harness.exit' +$TRIPWIRE_CHECK mkdir '$WT_PATH/.harness.done' 2>/dev/null exit \$CODE " @@ -380,6 +390,9 @@ fi # ── Launch detached + record pid + pgid ───────────────────────────────────── rm -f "$WT_PATH/.harness.exit" rm -rf "$WT_PATH/.harness.done" +# Issue #136 Part B: snapshot the MAIN checkout BEFORE the worker starts; +# the wrapper diffs against this right after the worker exits. +bash "$HARNESS_ROOT/scripts/check-main-tree-clean.sh" worker snapshot "$WT_PATH" || echo "run-worker: warning: main-tree tripwire snapshot failed" >&2 # A worktree is reused across continuation/rework respawns, so harness.log # must be APPENDED to, never truncated — a worker that hits the iteration cap # or gets a review rework cycle keeps its full log history (and the orchestrator diff --git a/src/orchestrator/__tests__/main-tree-tripwire.test.ts b/src/orchestrator/__tests__/main-tree-tripwire.test.ts new file mode 100644 index 0000000..a9aad06 --- /dev/null +++ b/src/orchestrator/__tests__/main-tree-tripwire.test.ts @@ -0,0 +1,243 @@ +import assert from "node:assert/strict" +import { execFileSync, spawnSync } from "node:child_process" +import * as fs from "node:fs" +import * as fsp from "node:fs/promises" +import * as os from "node:os" +import * as path from "node:path" +import { fileURLToPath } from "node:url" + +import { removeHarnessArtifacts } from "../cleanup.js" +import { buildStatusSummary, formatStatusText } from "../status.js" +import { reconcileGroups } from "../status.js" +import { + DEFAULT_STALL_TIMEOUT_MS, + inspectGroup, + readMainTreeDirty, +} from "../watch.js" + +const SCRIPT = fileURLToPath( + new URL("../../../scripts/check-main-tree-clean.sh", import.meta.url), +) + +function git(repo: string, args: string[]): string { + return execFileSync("git", ["-C", repo, ...args], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "ignore"], + }).trim() +} + +async function initRepo(repo: string): Promise { + execFileSync("git", ["init", "-q", "-b", "main", repo], { + stdio: "ignore", + }) + git(repo, ["config", "user.email", "test@headlesscode.invalid"]) + git(repo, ["config", "user.name", "Tripwire Test"]) + git(repo, ["config", "commit.gpgsign", "false"]) + await fsp.writeFile(path.join(repo, "README.md"), "# fixture\n") + git(repo, ["add", "README.md"]) + git(repo, ["commit", "-qm", "init"]) +} + +interface Fixture { + repo: string + wt: string +} + +async function makeFixture(): Promise { + const repo = await fsp.mkdtemp(path.join(os.tmpdir(), "hc-tripwire-")) + await initRepo(repo) + const wt = path.join(repo, ".worktrees", "w1") + git(repo, ["worktree", "add", "-q", "-b", "feat", wt, "main"]) + return { repo, wt } +} + +async function withFixture(fn: (fx: Fixture) => Promise): Promise { + const fx = await makeFixture() + try { + await fn(fx) + } finally { + git(fx.repo, ["worktree", "remove", "--force", fx.wt]) + git(fx.repo, ["worktree", "prune"]) + await fsp.rm(fx.repo, { recursive: true, force: true }) + } +} + +function tripwire(role: string, mode: string, wt: string): string { + const ran = spawnSync("bash", [SCRIPT, role, mode, wt], { + encoding: "utf-8", + }) + assert.equal(ran.status, 0, "tripwire always exits 0") + return `${ran.stdout}${ran.stderr}` +} + +function dirtyMarker(fx: Fixture, role: string): string { + const stem = role === "qa" ? ".qa" : ".harness" + return path.join(fx.wt, `${stem}.main-tree-dirty`) +} + +async function cleanRunLeavesNoMarker(fx: Fixture): Promise { + tripwire("worker", "snapshot", fx.wt) + tripwire("worker", "check", fx.wt) + assert.equal(fs.existsSync(dirtyMarker(fx, "worker")), false) +} + +async function escapedWriteIsReported(fx: Fixture): Promise { + tripwire("worker", "snapshot", fx.wt) + await fsp.writeFile(path.join(fx.repo, "escaped.txt"), "x\n") + await fsp.appendFile(path.join(fx.repo, "README.md"), "dirt\n") + const out = tripwire("worker", "check", fx.wt) + const marker = dirtyMarker(fx, "worker") + assert.equal(fs.existsSync(marker), true) + const body = await fsp.readFile(marker, "utf-8") + assert.ok(body.includes("escaped.txt")) + assert.ok(body.includes("README.md")) + assert.ok(out.includes("MAIN-TREE-DIRTY")) +} + +async function orchestratorDirsExcluded(fx: Fixture): Promise { + tripwire("worker", "snapshot", fx.wt) + const churn = path.join(fx.repo, "plans", "parallel-tasks") + await fsp.mkdir(churn, { recursive: true }) + await fsp.writeFile(path.join(churn, "churn.md"), "x\n") + const sib = path.join(fx.repo, ".worktrees", "other") + await fsp.mkdir(sib, { recursive: true }) + await fsp.writeFile(path.join(sib, "x.txt"), "x\n") + tripwire("worker", "check", fx.wt) + assert.equal(fs.existsSync(dirtyMarker(fx, "worker")), false) +} + +async function missingBaselineReportsAll(fx: Fixture): Promise { + await fsp.writeFile(path.join(fx.repo, "stale.txt"), "x\n") + const out = tripwire("worker", "check", fx.wt) + assert.ok(out.includes("no baseline")) + const body = await fsp.readFile(dirtyMarker(fx, "worker"), "utf-8") + assert.ok(body.includes("stale.txt")) +} + +async function mainCheckoutRefusesLoudly(fx: Fixture): Promise { + const out = tripwire("worker", "snapshot", fx.repo) + assert.ok(out.includes("directly on")) + const before = path.join(fx.repo, ".harness.main-tree-before") + assert.equal(fs.existsSync(before), false) +} + +async function qaRoleUsesQaMarkers(fx: Fixture): Promise { + tripwire("qa", "snapshot", fx.wt) + await fsp.writeFile(path.join(fx.repo, "qa-escaped.txt"), "x\n") + tripwire("qa", "check", fx.wt) + assert.equal(fs.existsSync(dirtyMarker(fx, "qa")), true) + assert.equal(fs.existsSync(dirtyMarker(fx, "worker")), false) +} + +async function scriptAndReaderAgree(fx: Fixture): Promise { + tripwire("worker", "snapshot", fx.wt) + await fsp.writeFile(path.join(fx.repo, "escape.txt"), "x\n") + tripwire("worker", "check", fx.wt) + assert.deepEqual(readMainTreeDirty(fx.wt), ["?? escape.txt"]) +} + +async function withTmpDir(fn: (dir: string) => Promise): Promise { + const dir = await fsp.mkdtemp(path.join(os.tmpdir(), "hc-tripwire-read-")) + try { + await fn(dir) + } finally { + await fsp.rm(dir, { recursive: true, force: true }) + } +} + +async function readerReadsMarkerLines(dir: string): Promise { + assert.equal(readMainTreeDirty(dir), undefined) + const wt = path.join(dir, "w1") + await fsp.mkdir(wt) + const marker = path.join(wt, ".harness.main-tree-dirty") + await fsp.writeFile(marker, "?? a.txt\n M b.txt\n\n") + assert.deepEqual(readMainTreeDirty(wt), ["?? a.txt", " M b.txt"]) +} + +async function inspectPersistsDirty(dir: string): Promise { + const wt = path.join(dir, ".worktrees", "w1") + await fsp.mkdir(wt, { recursive: true }) + await fsp.mkdir(path.join(wt, ".harness.done")) + await fsp.writeFile(path.join(wt, ".harness.exit"), "0") + const marker = path.join(wt, ".harness.main-tree-dirty") + await fsp.writeFile(marker, "?? escaped.txt\n") + const group = { name: "w1", status: "running" } + const patch = inspectGroup(dir, group, Date.now(), DEFAULT_STALL_TIMEOUT_MS) + assert.deepEqual(patch?.main_tree_dirty, ["?? escaped.txt"]) + const la = patch?.last_activity as { note: string } + assert.match(la.note, /MAIN-TREE-DIRTY/) +} + +async function reconcilePersistsDirty(dir: string): Promise { + const wt = path.join(dir, ".worktrees", "w1") + await fsp.mkdir(wt, { recursive: true }) + await fsp.mkdir(path.join(wt, ".harness.done")) + await fsp.writeFile(path.join(wt, ".harness.exit"), "0") + const marker = path.join(wt, ".harness.main-tree-dirty") + await fsp.writeFile(marker, " M escaped.txt\n") + const groups = [{ name: "w1", status: "running" }] + const { state: out } = reconcileGroups({ batch: "t", groups }, dir) + assert.deepEqual(out.groups[0]?.main_tree_dirty, [" M escaped.txt"]) +} + +async function summarySurfacesDirtyBit(dir: string): Promise { + const groups = [{ name: "w1", status: "done", main_tree_dirty: ["?? e.txt"] }] + const summary = buildStatusSummary({ batch: "t", groups }) + assert.deepEqual(summary.groups[0]?.mainTreeDirty, ["?? e.txt"]) + const text = formatStatusText(summary, { repo: dir, statePath: "s" }) + assert.match(text, /MAIN-TREE-DIRTY/) +} + +async function cleanupRemovesMarkers(dir: string): Promise { + const names = [ + ".harness.main-tree-before", + ".harness.main-tree-dirty", + ".qa.main-tree-before", + ".qa.main-tree-dirty", + ] + for (const name of names) await fsp.writeFile(path.join(dir, name), "x") + removeHarnessArtifacts(dir) + for (const name of names) { + assert.equal(fs.existsSync(path.join(dir, name)), false) + } +} + +const tests: Array<[string, (fx: Fixture) => Promise]> = [ + ["clean run leaves no marker", cleanRunLeavesNoMarker], + ["escaped write is reported", escapedWriteIsReported], + ["orchestrator dirs excluded", orchestratorDirsExcluded], + ["missing baseline reports all", missingBaselineReportsAll], + ["main checkout refuses loudly", mainCheckoutRefusesLoudly], + ["qa role uses qa markers", qaRoleUsesQaMarkers], + ["script and reader agree", scriptAndReaderAgree], + ["reader reads marker lines", () => withTmpDir(readerReadsMarkerLines)], + ["inspect persists dirty", () => withTmpDir(inspectPersistsDirty)], + ["reconcile persists dirty", () => withTmpDir(reconcilePersistsDirty)], + ["summary surfaces dirty bit", () => withTmpDir(summarySurfacesDirtyBit)], + ["cleanup removes markers", () => withTmpDir(cleanupRemovesMarkers)], +] + +async function main(): Promise { + let failed = 0 + for (const [name, fn] of tests) { + try { + await withFixture(fn) + console.log(` ok ${name}`) + } catch (err) { + failed++ + console.error(` FAIL ${name}`) + const detail = err instanceof Error ? err.stack ?? err.message : String(err) + console.error(detail) + } + } + if (failed > 0) { + console.error(`\n${failed} test(s) failed`) + process.exit(1) + } + console.log(`\nAll ${tests.length} main-tree-tripwire tests passed`) +} + +main().catch((err) => { + console.error("Test runner crashed:", err) + process.exit(1) +}) diff --git a/src/orchestrator/cleanup.ts b/src/orchestrator/cleanup.ts index a064ef2..e10abce 100644 --- a/src/orchestrator/cleanup.ts +++ b/src/orchestrator/cleanup.ts @@ -84,6 +84,10 @@ const HARNESS_ARTIFACTS = [ ".harness.done", ".harness.needs-decision", ".harness.decision-answer", + ".harness.main-tree-before", // issue #136 Part B tripwire baseline + ".harness.main-tree-dirty", // issue #136 Part B tripwire finding + ".qa.main-tree-before", + ".qa.main-tree-dirty", ".qa.pid", ".qa.exit", ".qa.done", diff --git a/src/orchestrator/state.ts b/src/orchestrator/state.ts index ecee9d4..d0ca17d 100644 --- a/src/orchestrator/state.ts +++ b/src/orchestrator/state.ts @@ -110,6 +110,13 @@ export interface OrchestratorGroup { pending_review_findings?: string[] /** Worker exit code (written by run-worker.sh; recorded by the watcher). */ exit_code?: number + /** + * Issue #136 Part B: main-checkout entries that appeared during this + * group's run window — the worker wrote outside its worktree. + * Persisted from /.harness.main-tree-dirty at completion; absent + * means a clean run (or a group that finished before the tripwire). + */ + main_tree_dirty?: string[] /** harness.log tail captured when the group completed. */ summary?: string /** Set by the watcher's stall guard. */ diff --git a/src/orchestrator/status.ts b/src/orchestrator/status.ts index 95c3d45..7e1e322 100644 --- a/src/orchestrator/status.ts +++ b/src/orchestrator/status.ts @@ -66,8 +66,10 @@ import { import { DEFAULT_POLL_INTERVAL_MS, DEFAULT_STALL_TIMEOUT_MS, + describeMainTreeDirty, groupWorktreePath, hasExitMarker, + readMainTreeDirty, workerLiveness, type WorkerLiveness, } from "./watch.js" @@ -103,6 +105,8 @@ export interface StatusGroupRow { findingsCount: number usageCostUsd?: number exitCode?: number + /** Issue #136 Part B tripwire finding (undefined = clean run). */ + mainTreeDirty?: string[] stalled: boolean blockedQuestion?: string qaVerdict?: string @@ -212,6 +216,7 @@ export function buildStatusSummary(state: OrchestratorState): StatusSummary { findingsCount: g.pending_review_findings?.length ?? 0, usageCostUsd: g.usage?.costUsd, exitCode: g.exit_code, + mainTreeDirty: g.main_tree_dirty, stalled: g.stalled === true, blockedQuestion: g.blocked?.question, qaVerdict: g.qa?.verdict, @@ -337,6 +342,9 @@ export function formatStatusText( if (g.stalled) { bits.push("STALLED") } + if (g.mainTreeDirty !== undefined) { + bits.push(describeMainTreeDirty(g.mainTreeDirty)) + } if (g.blockedQuestion) { bits.push(`blocked: ${g.blockedQuestion}`) } @@ -381,6 +389,11 @@ export interface ReconcileHooks { hasExitMarker?: (worktreePath: string) => boolean /** Worker liveness (default: workerLiveness from watch.js). */ workerLiveness?: (worktreePath: string) => WorkerLiveness + /** + * `.harness.main-tree-dirty` reader. + * Default: readMainTreeDirty from watch.js. + */ + readMainTreeDirty?: (worktreePath: string) => string[] | undefined } export interface ReconcileResult { @@ -426,12 +439,18 @@ export function reconcileGroups(state: OrchestratorState, repo: string, hooks: R }) const exitPresent = hooks.hasExitMarker ?? hasExitMarker const livenessOf = hooks.workerLiveness ?? workerLiveness + const readDirtyMarker = hooks.readMainTreeDirty ?? readMainTreeDirty let current = state const reconciled: string[] = [] for (const group of state.groups) { const patch = reconcileOneGroup(group, repo, { - worktreeExists, hasDoneMarker, readExitCode, exitPresent, livenessOf, + worktreeExists, + hasDoneMarker, + readExitCode, + exitPresent, + livenessOf, + readMainTreeDirty: readDirtyMarker, }) if (patch) { current = updateGroup(current, group.name, patch) @@ -447,23 +466,45 @@ interface ReconcileProbes { readExitCode: (p: string) => number | undefined exitPresent: (p: string) => boolean livenessOf: (p: string) => WorkerLiveness + readMainTreeDirty: (p: string) => string[] | undefined +} + +function tripwirePatch( + probes: ReconcileProbes, + wtPath: string, +): { extra: Partial; suffix: string } { + const dirty = probes.readMainTreeDirty(wtPath) + if (dirty === undefined) { + return { extra: {}, suffix: "" } + } + const suffix = `; ${describeMainTreeDirty(dirty)}` + return { extra: { main_tree_dirty: dirty }, suffix } +} + +function reconcileDoneNote( + exitCode: number | undefined, + suffix: string, +): string { + const base = + exitCode === 0 + ? "reconciled: worker completed cleanly (exit 0)" + : `reconciled: worker exited ${exitCode ?? "with unknown code"}` + return base + suffix } function doneMarkerPatch( probes: ReconcileProbes, wtPath: string, ): Partial { - // Ground truth exists: the worker finished. Mirror watch.ts's - // mapping (exit 0 → done, anything else/unknown → failed). + // Mirror watch.ts: exit 0 → done, anything else/unknown → failed. const exitCode = probes.readExitCode(wtPath) - const status = exitCode === 0 ? "done" : "failed" + const trip = tripwirePatch(probes, wtPath) return { - status, + status: exitCode === 0 ? "done" : "failed", ...(exitCode !== undefined ? { exit_code: exitCode } : {}), + ...trip.extra, last_activity: { - note: exitCode === 0 - ? "reconciled: worker completed cleanly (exit 0)" - : `reconciled: worker exited ${exitCode ?? "with unknown code"}`, + note: reconcileDoneNote(exitCode, trip.suffix), }, } } @@ -484,11 +525,14 @@ function killedPatch( ): Partial { const exitCode = probes.readExitCode(wtPath) const detail = exitCode ?? "unparseable" + const trip = tripwirePatch(probes, wtPath) + const baseNote = `reconciled: worker dead with exit ${detail}, no .done marker` return { status: "failed", ...(exitCode !== undefined ? { exit_code: exitCode } : {}), + ...trip.extra, last_activity: { - note: `reconciled: worker dead with exit ${detail}, no .done marker`, + note: baseNote + trip.suffix, }, } } diff --git a/src/orchestrator/watch.ts b/src/orchestrator/watch.ts index edab945..168d7f4 100644 --- a/src/orchestrator/watch.ts +++ b/src/orchestrator/watch.ts @@ -203,6 +203,31 @@ function readExitCode(wtPath: string): number | undefined { } } +/** + * Issue #136 Part B: read the main-tree tripwire marker written by + * scripts/check-main-tree-clean.sh — the main-checkout entries that + * appeared during this worker's run window. Porcelain lines, kept + * verbatim (the XY prefix is evidence). Undefined when the marker is + * absent or empty (clean run, or a worktree from before the tripwire). + */ +export function readMainTreeDirty(wtPath: string): string[] | undefined { + try { + const marker = path.join(wtPath, ".harness.main-tree-dirty") + const raw = fs.readFileSync(marker, "utf-8") + const entries = raw.split("\n").filter((line) => line !== "") + return entries.length > 0 ? entries : undefined + } catch { + return undefined + } +} + +/** One loud line describing a tripwire finding (file list capped). */ +export function describeMainTreeDirty(entries: string[]): string { + const shown = entries.slice(0, 20).join(", ") + const more = entries.length > 20 ? `, +${entries.length - 20} more` : "" + return `MAIN-TREE-DIRTY: worker wrote outside its worktree (${shown}${more})` +} + /** * Cost/token monitoring (workstream 3): sum every usage record found under * `/.headlesscode/usage/*.jsonl` (the worker's session usage — see @@ -474,6 +499,9 @@ export function inspectGroup( const exitCode = readExitCode(wtPath) const status = exitCode === 0 ? "done" : "failed" const summary = tailLog(path.join(wtPath, "harness.log")) + const dirty = readMainTreeDirty(wtPath) + const dirtyNote = + dirty === undefined ? "" : `; ${describeMainTreeDirty(dirty)}` const note = exitCode === 0 ? `worker completed cleanly (exit 0)${summary ? " — see harness.log" : ""}` @@ -482,11 +510,12 @@ export function inspectGroup( return { status, last_activity: { - note, + note: note + dirtyNote, last_commit: lastCommit(wtPath), }, commits: group.commits ?? [], ...(exitCode !== undefined ? { exit_code: exitCode } : {}), + ...(dirty === undefined ? {} : { main_tree_dirty: dirty }), // `.slice(-4000)`, NOT `.slice(0, 4000)`: this is a TAIL (already the // last SUMMARY_TAIL_LINES lines of the run) being capped to a char // budget. Keeping the FIRST 4000 chars of a tail throws away the most diff --git a/src/permissions/__tests__/commands.test.ts b/src/permissions/__tests__/commands.test.ts index 629dd9f..1335c74 100644 --- a/src/permissions/__tests__/commands.test.ts +++ b/src/permissions/__tests__/commands.test.ts @@ -15,11 +15,15 @@ import * as os from "node:os" import * as path from "node:path" import { + checkCdEscape, checkCommand, checkRedirectEscape, + checkWriteOperandEscape, containsDangerousSubstitution, decideCommand, + describeCdEscape, describeRedirect, + describeWriteEscape, findLongestPrefixMatch, getCommandDecision, isOutsideWorkspace, @@ -352,6 +356,199 @@ async function testRedirectEscapeNotOverridableByAllowList(): Promise { } } +// ─── cd-escape + write-operand confinement (issue #136) ───────────────────── + +function assertCdRefused(cmd: string, ws: string): void { + assert.notEqual(checkCdEscape(cmd, ws), null, `cd refuses: ${cmd}`) + assert.equal(decideCommand(cmd, [], [], { workspaceRoot: ws }), "deny") +} + +function assertCdAllowed(cmd: string, ws: string): void { + assert.equal(checkCdEscape(cmd, ws), null, `cd allows: ${cmd}`) +} + +function assertOperandRefused(cmd: string, ws: string): void { + const hit = checkWriteOperandEscape(cmd, ws) + assert.notEqual(hit, null, `operands refuse: ${cmd}`) + assert.equal(decideCommand(cmd, [], [], { workspaceRoot: ws }), "deny") +} + +function assertOperandAllowed(cmd: string, ws: string): void { + const hit = checkWriteOperandEscape(cmd, ws) + assert.equal(hit, null, `operands allow: ${cmd}`) +} + +function assertDecided(cmd: string, ws: string, verdict: string): void { + assert.equal(decideCommand(cmd, [], [], { workspaceRoot: ws }), verdict, cmd) +} + +function assertRefusedAs(cmd: string, ws: string, kind: string): void { + const refusal = checkCommand(cmd, [], [], { workspaceRoot: ws }) + assert.ok(refusal !== null && refusal.kind === kind, `${cmd} -> ${kind}`) +} + +async function withTempWs( + label: string, + fn: (ws: string) => void, +): Promise { + const ws = await fs.mkdtemp(path.join(os.tmpdir(), `hc-cmd-${label}-`)) + try { + fn(ws) + } finally { + await fs.rm(ws, { recursive: true, force: true }) + } +} + +const CD_REFUSED_CASES = [ + `cd /tmp`, `cd /`, `cd ..`, `cd ../..`, `cd ~/x`, `cd ~`, `cd`, `cd ""`, + `cd /tmp && echo hi`, `(cd /tmp)`, `(cd /tmp && make)`, `{ cd /tmp; }`, + `pushd /tmp`, `cd -- /tmp`, `FOO=1 cd /tmp`, `env FOO=1 cd /tmp`, + `command cd /tmp`, `builtin cd /tmp`, `time cd /tmp`, `! cd /tmp`, + `if cd /tmp; then x; fi`, `cd $HOME`, `cd $HC_UNSET_XYZ`, + `cd $HC_UNSET_XYZ/sub`, `bash -c 'cd /tmp'`, `bash -c "cd /tmp"`, + `sh -c 'cd /tmp'`, `eval "cd /tmp"`, `echo $(cd /tmp)`, + "echo `cd /tmp`", `echo $(echo hi && cd /tmp)`, `timeout 5 cd /tmp`, + `nice cd /tmp`, `nohup cd /tmp`, `until cd /tmp; do x; done`, + `env -i cd sub`, `env -u HOME cd ~`, +] + +const CD_ALLOWED_CASES = [ + `cd sub`, `cd .`, `cd ./sub`, `cd sub/dir`, `cd -`, `popd`, `pushd`, + `pushd +1`, `pushd -0`, `pushd sub`, `popd +1`, + `cd sub && echo hi > x`, `(cd sub && make)`, `{ cd sub; }`, + `FOO=1 cd sub`, `env FOO=1 cd sub`, `command cd sub`, + `cd $HC_TEST_SUBDIR`, `export D=sub && cd $D`, `D=sub && cd $D`, + `bash -c 'cd sub'`, `eval "cd sub"`, `echo $(pwd)`, `echo cd /tmp`, + `echo "cd /tmp"`, `cat < $O/f`, `O=d && echo hi > $O/f`, + `export A=1 B=sub && cd $B`, `export O=sub && cp a $O/b`, + `echo hi > \${HC_UNSET_XYZ:-sub}/f`, + `echo hi > $HC_UNSET_XYZ`, `rm -rf $HC_UNSET_XYZ`, +] + +const CHAIN_DENY_CASES: Array<[string, string]> = [ + [`export O=/tmp && echo hi > $O/f`, "redirect_escape"], + [`F=$(mktemp -d) && echo hi > $F/x`, "redirect_escape"], + ["F=`mktemp` && cp a $F", "write_escape"], + [`export D=/tmp && cd $D`, "cd_escape"], + [`echo hi > \${HC_UNSET_XYZ:-/tmp/x}`, "redirect_escape"], + [`echo hi > $HC_UNSET_XYZ/x`, "redirect_escape"], + [`rm -rf $HC_UNSET_XYZ/`, "protected_store"], +] + +async function testCdEscapeRefused(): Promise { + await withTempWs("cd", (ws) => { + for (const cmd of CD_REFUSED_CASES) assertCdRefused(cmd, ws) + }) +} + +async function testCdEscapeAllowed(): Promise { + const saved = process.env.HC_TEST_SUBDIR + process.env.HC_TEST_SUBDIR = "sub/dir" + try { + await withTempWs("cd-ok", (ws) => { + for (const cmd of CD_ALLOWED_CASES) assertCdAllowed(cmd, ws) + }) + } finally { + if (saved === undefined) delete process.env.HC_TEST_SUBDIR + else process.env.HC_TEST_SUBDIR = saved + } +} + +async function testCdEscapeCdpath(): Promise { + const saved = process.env.CDPATH + try { + await withTempWs("cdpath", (ws) => { + delete process.env.CDPATH + assertCdAllowed(`cd foo`, ws) + process.env.CDPATH = "/tmp" + assertCdRefused(`cd foo`, ws) + assertCdAllowed(`cd ./foo`, ws) + assertCdAllowed(`cd sub/foo`, ws) + }) + } finally { + if (saved === undefined) delete process.env.CDPATH + else process.env.CDPATH = saved + } +} + +async function testWriteOperandRefused(): Promise { + await withTempWs("op", (ws) => { + for (const cmd of OP_REFUSED_CASES) assertOperandRefused(cmd, ws) + }) +} + +async function testWriteOperandAllowed(): Promise { + await withTempWs("op-ok", (ws) => { + for (const cmd of OP_ALLOWED_CASES) assertOperandAllowed(cmd, ws) + }) +} + +async function testChainExportOverlay(): Promise { + await withTempWs("chain", (ws) => { + for (const cmd of CHAIN_ALLOW_CASES) assertDecided(cmd, ws, "allow") + for (const [cmd, kind] of CHAIN_DENY_CASES) assertRefusedAs(cmd, ws, kind) + }) +} + +async function testCdRefusalPrecedesRedirect(): Promise { + await withTempWs("order", (ws) => { + const cdFirst = [ + `cd /tmp && echo hi > x`, `(cd /tmp && echo hi > x)`, `cd && echo hi > x`, + ] + for (const cmd of cdFirst) assertRefusedAs(cmd, ws, "cd_escape") + const probe = `cd /tmp && echo hi > x` + const refusal = checkCommand(probe, [], [], { workspaceRoot: ws }) + assert.ok(refusal !== null && refusal.cd !== undefined) + assertDecided(`cd sub && echo hi > x`, ws, "allow") + const target = { subCommand: "cd /tmp", directory: "/tmp" } + assert.equal(describeCdEscape(target), "cd /tmp") + const wrote = checkWriteOperandEscape(`cp a /tmp/b`, ws) + assert.ok(wrote !== null && wrote.command === "cp") + assert.ok(wrote !== null && wrote.operand === "/tmp/b") + const operand = { subCommand: "x", command: "cp", operand: "/tmp/b" } + assert.equal(describeWriteEscape(operand), "cp writes /tmp/b") + }) +} + // ─── Runner ────────────────────────────────────────────────────────────────── const tests: Array<[string, () => Promise]> = [ @@ -364,6 +561,13 @@ const tests: Array<[string, () => Promise]> = [ ["redirect-escape guard (issue #122): >/>>/2>/&> outside the workspace always denied, inside allowed", testRedirectEscapeAlwaysDenied], ["redirect target extraction + resolution + describeRedirect", testRedirectTargetParsing], ["redirect-escape guard is NOT overridable by an allow-everything config", testRedirectEscapeNotOverridableByAllowList], + ["cd outside the workspace is refused (#136)", testCdEscapeRefused], + ["cd inside the workspace is allowed", testCdEscapeAllowed], + ["cd with CDPATH fails closed on slashless operands", testCdEscapeCdpath], + ["write operands outside the workspace are refused", testWriteOperandRefused], + ["write operands inside the workspace are allowed", testWriteOperandAllowed], + ["chain exports overlay; unknown vars fail closed", testChainExportOverlay], + ["cd refusal precedes redirect; describes", testCdRefusalPrecedesRedirect], ] async function main(): Promise { diff --git a/src/permissions/__tests__/store-protection.test.ts b/src/permissions/__tests__/store-protection.test.ts index 8a58815..24ae19b 100644 --- a/src/permissions/__tests__/store-protection.test.ts +++ b/src/permissions/__tests__/store-protection.test.ts @@ -196,11 +196,23 @@ async function testSec6DescendantsSymlinksCdChains(): Promise { assert.ok(symlinkRefusal !== null, "rm -rf through a symlink to the store must be refused") assert.equal(symlinkRefusal?.kind, "protected_store") - // cd-chain: a relative target resolves against the post-cd cwd, not the - // pre-cd workspaceRoot. - const cdChainRefusal = checkCommand(`cd ${tmp} && rm -rf central-store`, [], [], { workspaceRoot: workspace }) - assert.ok(cdChainRefusal !== null, "cd && rm -rf central-store must resolve against the post-cd cwd") - assert.equal(cdChainRefusal?.kind, "protected_store") + // cd-chain outside the workspace refuses AT the cd (cd_escape precedes + // the store check — the cd itself already violates confinement, so the + // rm is never reached). Same blocking outcome, earlier + more precise. + const outsideCd = `cd ${tmp} && rm -rf central-store` + const decided = { workspaceRoot: workspace } + const cdChainRefusal = checkCommand(outsideCd, [], [], decided) + assert.ok(cdChainRefusal !== null, "outside cd must be refused at the cd") + assert.equal(cdChainRefusal?.kind, "cd_escape") + + // Inside-only cd chains still resolve later targets against the + // post-cd cwd: a relative climb from the workspace to the store is + // caught by the store check, not the cd check. + const relToStore = path.relative(workspace, storeRoot) + const climb = `cd ${workspace} && rm -rf ${relToStore}` + const trackedRefusal = checkCommand(climb, [], [], decided) + assert.ok(trackedRefusal !== null, "relative climb must resolve post-cd") + assert.equal(trackedRefusal?.kind, "protected_store") // cd-chain into an unrelated dir does not false-positive. const cdChainAllowed = checkCommand(`cd ${workspace} && rm -rf ./scratch`, [], [], { workspaceRoot: workspace }) @@ -328,11 +340,13 @@ async function testWordSplittingAndPathResolution(): Promise { assert.equal(resolveCommandTarget("./scratch", "/ws"), path.resolve("/ws", "./scratch")) assert.equal(resolveCommandTarget("~/x", "/ws"), path.join(os.homedir(), "x")) assert.equal(resolveCommandTarget("/abs/x", "/ws"), "/abs/x", "absolute targets ignore the workspace root") - // With the store override UNSET, $HEADLESSCODE_DATA_DIR stays literal (an - // unset var is not expanded). `npm test` sets the var at the suite level, - // so clear it explicitly for this deterministic assertion. + // With the store override UNSET, $HEADLESSCODE_DATA_DIR strips to empty + // (exactly what the shell expands it to — bare `$U` then errors, safe). + // `npm test` sets the var at the suite level, so clear it explicitly for + // this deterministic assertion. await withEnv("HEADLESSCODE_DATA_DIR", undefined, () => { - assert.equal(resolveCommandTarget("$HEADLESSCODE_DATA_DIR", "/ws"), path.resolve("/ws", "$HEADLESSCODE_DATA_DIR")) + const stripped = resolveCommandTarget("$HEADLESSCODE_DATA_DIR", "/ws") + assert.equal(stripped, path.resolve("/ws")) }) // Containment edges (no filesystem access — pure path comparison). diff --git a/src/permissions/commands.ts b/src/permissions/commands.ts index ad6ea21..a95f6f4 100644 --- a/src/permissions/commands.ts +++ b/src/permissions/commands.ts @@ -53,6 +53,14 @@ * that writes outside the workspace via a non-redirect mechanism (e.g. * `python -c "open('/tmp/x','w')"`) is out of scope; see * redirectTargets/checkRedirectEscape below. + * - CD-ESCAPE + WRITE-OPERAND PROTECTION (always-applied, NOT configurable, + * issue #136): a `cd`/`pushd` outside the workspace, or a file-writing + * operand (cp/mv/ln/install/rsync/scp destination, `tee` output, `git` + * repo pointer, `tar`/`sed -i`/`curl`/`wget` output, shell script file) + * outside the workspace, is refused. The cd guard is load-bearing: an + * outside-cd re-anchors every later relative path, defeating the other + * guards. Same honest scope as above (sudo/ssh/xargs/script contents + * are boundaries); see checkCdEscape/checkWriteOperandEscape below. * * `parseCommand` is ported WITHOUT the `shell-quote` dependency (not present in * this repo's node_modules): quoted strings, arithmetic, parameter expansions, @@ -69,7 +77,17 @@ import * as os from "node:os" import * as path from "node:path" -import { checkCentralStoreDestruction, expandEnv, expandHome, splitCommandWords } from "./store-protection.js" +import { + type ChainEnv, + checkCentralStoreDestruction, + expandOperandWord, + harvestSubAssignments, + newChainEnv, + prePoisonRawAssignments, + resolveOperandTarget, + splitCommandWords, + usesPoisonedVar, +} from "./store-protection.js" // ─── parseCommand port (zoo-code/src/shared/parse-command.ts) ─────────────── @@ -457,7 +475,12 @@ function parseCommandLine(command: string): string[] { const variables: string[] = [] const parameterExpansions: string[] = [] - let processedCommand = command.replace(/\d*>&\d*/g, (match) => { + // Noclobber redirect operators (`>|`, `>>|`, `&>|`, `&>>|`, with optional + // fd digits) mask like fd-duplications: their `|` is not a pipe and must + // not split the sub-command (else `cat >| /tmp/x` splits into `cat >` + // and `/tmp/x`, and the redirect guard never sees its target). + const redirMask = /\d*>&\d*|\d*(?:&>>\||&>\||>>\||>\|)/g + let processedCommand = command.replace(redirMask, (match) => { redirections.push(match) return `__REDIR_${redirections.length - 1}__` }) @@ -675,22 +698,12 @@ export function redirectTargets(command: string): RedirectTarget[] { /** * Resolve a redirect target word to an absolute path, anchored on the command's - * working directory. Applies the same `~`/`$VAR` expansion and `/*`-glob - * truncation as the central-store check (store-protection.ts's - * resolveCommandTarget), so `/tmp` and `/tmp/x` behave identically to `~` and - * `$TMPDIR` — a relative target like `> out.txt` anchors inside the workspace. - * - * A word the shell could expand (e.g. `$TMPDIR/x.txt`) resolves through the - * SAME expansions the central-store check applies (expandHome/expandEnv), so - * `> $TMPDIR/x` with TMPDIR set to /tmp refuses like the literal `/tmp/x` it - * expands to. An UNKNOWN variable stays literal — the shell would expand it - * to an empty word (the redirect would then hit a missing-operand error), - * which cannot write outside the workspace, so resolving it literally is the - * safe direction. + * working directory. Delegates to the shared operand resolver: known vars + * expand, unknown vars strip (exact — unset vars expand to empty, so + * `> $U/x` really writes `/x` while bare `> $U` really errors, safe). */ export function resolveRedirectTarget(target: string, workspaceRoot: string): string { - const expanded = expandEnv(expandHome(target)) - return path.resolve(workspaceRoot, expanded) + return resolveOperandTarget(target, path.resolve(workspaceRoot)) } /** @@ -718,20 +731,14 @@ export function isOutsideWorkspace(root: string, target: string): boolean { } /** - * Check ONE sub-command (already split by parseCommand) for an output - * redirect whose resolved target escapes the workspace root. Returns the - * first offending redirect, or `null` when every redirect target stays inside - * the workspace. `workspaceRoot` anchors relative targets (defaults to - * process.cwd()). + * Check a command for an output redirect escaping the workspace root. + * Runs the shared ordered walk: relative targets resolve against the + * tracked cd-chain cwd but containment compares against the workspace + * root (the old code compared against the post-cd directory, so + * `cd /tmp && echo hi > x` wrongly passed). Chain exports overlay. */ export function checkRedirectEscape(subCommand: string, workspaceRoot?: string): RedirectTarget | null { - const root = path.resolve(workspaceRoot ?? process.cwd()) - for (const target of redirectTargets(subCommand)) { - if (isOutsideWorkspace(root, resolveRedirectTarget(target.word, root))) { - return target - } - } - return null + return scanWholeCommand(subCommand, workspaceRoot).redir } /** Human-readable operator for the model-facing refusal message. */ @@ -741,6 +748,1001 @@ export function describeRedirect(target: RedirectTarget): string { return compact ? `${target.operator}${target.word}` : `${target.operator} ${target.word}` } +// ─── workspace-confinement scans (issue #136) ── +// +// A `cd` outside the workspace re-anchors every later relative path in the +// chain, silently defeating the redirect/operand guards (the SEC-6 tracking +// resolved AND compared against the post-cd directory — always "inside"). +// The load-bearing fix is refusing the outside-cd itself; the redirect and +// write-operand scans are backstops for absolute paths. All three scans +// share one ordered sub-command walk so chain exports, cd tracking, and +// first-hit-wins stay consistent between checkCommand and the exported +// single-purpose checks. This is pattern-based, not a sandbox: sudo/su, +// ssh, xargs/find -exec command construction, and script-file CONTENTS are +// documented boundaries (an allowed `sudo` already moots confinement). + +export interface CdEscape { + subCommand: string + directory: string + reason?: string +} + +export interface WriteOperandEscape { + subCommand: string + command: string + operand: string + reason?: string +} + +export function describeCdEscape(escape: CdEscape): string { + const base = `cd ${escape.directory}` + return escape.reason === undefined ? base : `${base} (${escape.reason})` +} + +export function describeWriteEscape(escape: WriteOperandEscape): string { + if (escape.reason !== undefined) { + return `${escape.command} ${escape.operand}: ${escape.reason}` + } + return `${escape.command} writes ${escape.operand}` +} + +/** Mutable state for one ordered sub-command walk. `contain` never moves + * (the workspace root); `cwd` tracks inside-only cds; `env` accumulates + * chain exports. Forks (subshells, `sh -c`) copy everything but share hits. */ +interface ScanCtx { + contain: string + cwd: string + prevCwd: string + stack: string[] + env: ChainEnv +} + +interface ScanOut { + cd: CdEscape | null + op: WriteOperandEscape | null + redir: RedirectTarget | null +} + +function newScanCtx(root: string): ScanCtx { + const resolved = path.resolve(root) + return { + contain: resolved, cwd: resolved, prevCwd: resolved, stack: [], + env: newChainEnv(), + } +} + +function hasMoreFlags(rest: string[]): boolean { + return rest.length > 0 && (rest[0] ?? "").startsWith("-") && rest[0] !== "--" +} + +function forkScanCtx(ctx: ScanCtx): ScanCtx { + return { + contain: ctx.contain, + cwd: ctx.cwd, + prevCwd: ctx.prevCwd, + stack: [...ctx.stack], + env: { overlay: new Map(ctx.env.overlay), poison: new Set(ctx.env.poison) }, + } +} + +function newScanOut(): ScanOut { + return { cd: null, op: null, redir: null } +} + +function mergeChainEnv(chain: ChainEnv, local: ChainEnv): ChainEnv { + return { + overlay: new Map([...chain.overlay, ...local.overlay]), + poison: new Set([...chain.poison, ...local.poison]), + } +} + +interface NormalizedArgv { + argv0: string + args: string[] + local: ChainEnv + opaqueEnv: boolean + opaqueFlag: string | null + parenLocal: boolean +} + +const SHELL_KEYWORDS = new Set([ + "!", "time", "command", "builtin", "if", "while", "until", + "do", "then", "else", "elif", +]) + +interface StrippedWrappers { + words: string[] + opaque: boolean + opaqueFlag: string | null + local: string[] +} + +function splitEnvPrefix(words: string[]): { env: string[]; rest: string[] } { + let i = 0 + while (i < words.length && /^[A-Za-z_][A-Za-z0-9_]*=/.test(words[i] ?? "")) i++ + return { env: words.slice(0, i), rest: words.slice(i) } +} + +interface GroupedWords { + words: string[] + parenLocal: boolean +} + +function stripGroupDelimiters(words: string[]): GroupedWords { + let rest = words + let parenLocal = false + while (rest.length > 0 && /^[{()]+$/.test(rest[0])) { + if (rest[0].includes("(")) parenLocal = true + rest = rest.slice(1) + } + if (rest.length > 0 && /^[(]/.test(rest[0])) { + parenLocal = true + rest = [rest[0].replace(/^[(]+/, ""), ...rest.slice(1)] + } + if (rest.length > 0 && /^[{}]/.test(rest[0])) { + rest = [rest[0].replace(/^[{}]+/, ""), ...rest.slice(1)] + } + return { words: rest, parenLocal } +} + +function timeoutFlagTakesValue(flag: string): boolean { + const valued = new Set(["-s", "-k", "--signal", "--kill-after"]) + return valued.has(flag) +} + +function stripTimeoutPrefix(words: string[]): string[] { + let rest = words.slice(1) + while (hasMoreFlags(rest)) { + rest = rest.slice(timeoutFlagTakesValue(rest[0] ?? "") ? 2 : 1) + } + if (rest[0] === "--") rest = rest.slice(1) + if (rest.length > 0 && !(rest[0] ?? "").startsWith("-")) rest = rest.slice(1) + return rest +} + +function stripNicePrefix(words: string[]): string[] { + let rest = words.slice(1) + while (hasMoreFlags(rest)) { + const flag = rest[0] ?? "" + rest = rest.slice(flag === "-n" || flag === "--adjustment" ? 2 : 1) + } + if (rest[0] === "--") rest = rest.slice(1) + return rest +} + +function envFlagIsOpaque(flag: string): boolean { + const opaque = new Set(["-i", "-u", "--ignore-environment", "--unset"]) + return opaque.has(flag) +} + +function stripEnvCommand(words: string[]): StrippedWrappers { + let rest = words.slice(1) + let opaque = false + let opaqueFlag: string | null = null + while (hasMoreFlags(rest)) { + const flag = rest[0] ?? "" + if (envFlagIsOpaque(flag)) { + opaque = true + opaqueFlag = flag + } + rest = rest.slice(flag === "-u" || flag === "--unset" ? 2 : 1) + } + if (rest[0] === "--") rest = rest.slice(1) + const prefix = splitEnvPrefix(rest) + return { words: prefix.rest, opaque, opaqueFlag, local: prefix.env } +} + +/** Peel one non-env transparent wrapper; null when the head is none. */ +function stripPlainWrapper(words: string[]): string[] | null { + const head = words[0] ?? "" + if (head === "time") { + const rest = words.slice(1) + return rest[0] === "-p" ? rest.slice(1) : rest + } + if (SHELL_KEYWORDS.has(head)) return words.slice(1) + if (head === "timeout") return stripTimeoutPrefix(words) + if (head === "nice") return stripNicePrefix(words) + if (head === "nohup") return words.slice(1) + return null +} + +interface WrapperAcc { + opaque: boolean + opaqueFlag: string | null + local: string[] +} + +function applyEnvWrapper(rest: string[], acc: WrapperAcc): string[] | null { + if ((rest[0] ?? "") !== "env") return null + const stripped = stripEnvCommand(rest) + acc.opaque = acc.opaque || stripped.opaque + acc.opaqueFlag = stripped.opaqueFlag ?? acc.opaqueFlag + acc.local.push(...stripped.local) + return stripped.words +} + +function stripCommandWrappers(words: string[]): StrippedWrappers { + let rest = words + const acc: WrapperAcc = { opaque: false, opaqueFlag: null, local: [] } + for (;;) { + const peeled = stripPlainWrapper(rest) ?? applyEnvWrapper(rest, acc) + if (peeled === null) break + rest = peeled + } + return { + words: rest, opaque: acc.opaque, opaqueFlag: acc.opaqueFlag, local: acc.local, + } +} + +/** Reduce split words to the effective argv: env prefixes (kept as a local + * overlay), grouping delimiters, and transparent wrappers (keywords, + * timeout/nice/nohup/env) are peeled; `sudo` is deliberately NOT peeled — + * an allowed sudo already moots confinement (operator's choice). */ +function normalizeArgv(words: string[]): NormalizedArgv | null { + const prefix = splitEnvPrefix(words) + const local = newChainEnv() + harvestSubAssignments(prefix.env, local) + const grouped = stripGroupDelimiters(prefix.rest) + const stripped = stripCommandWrappers(grouped.words) + harvestSubAssignments(stripped.local, local) + const core = stripped.words.filter((word) => word !== "") + if (core.length === 0) return null + return { + argv0: path.basename(core[0] ?? ""), + args: core.slice(1), + local, + opaqueEnv: stripped.opaque, + opaqueFlag: stripped.opaqueFlag, + parenLocal: grouped.parenLocal, + } +} + +/** First non-flag word of a cd/pushd argv (`-` survives: it toggles cwd). */ +function cdDirOperand(args: string[]): string | null { + let rest = args + while (hasMoreFlags(rest) && rest[0] !== "-") rest = rest.slice(1) + if (rest[0] === "--") rest = rest.slice(1) + return rest[0] ?? null +} + +function cdpathBlocks(dir: string, subEnv: ChainEnv): boolean { + if (dir.startsWith("~") || dir.includes("/") || dir === "-") return false + const cdpath = subEnv.overlay.get("CDPATH") ?? process.env.CDPATH ?? "" + return cdpath !== "" +} + +function homeEscape( + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): CdEscape | null { + const resolved = resolveOperandTarget("~", ctx.cwd, subEnv) + if (!isOutsideWorkspace(ctx.contain, resolved)) return null + return { subCommand: sub, directory: "~" } +} + +function opaqueEnvCdEscape(norm: NormalizedArgv, sub: string): CdEscape { + return { + subCommand: sub, + directory: norm.opaqueFlag ?? "env", + reason: "cleared environment is opaque", + } +} + +function cdCheckWord(word: string, subEnv: ChainEnv): string { + if (word === "") return "~" + return expandOperandWord(word, subEnv) === "" ? "~" : word +} + +function poisonedCdHit( + word: string, + sub: string, + subEnv: ChainEnv, +): CdEscape | null { + if (!usesPoisonedVar(word, subEnv)) return null + return { subCommand: sub, directory: word, reason: "unresolvable variable" } +} + +function containedCdHit( + check: string, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): CdEscape | null { + if (cdpathBlocks(check, subEnv)) { + const reason = "CDPATH makes slashless cd unresolvable" + return { subCommand: sub, directory: check, reason } + } + const resolved = resolveOperandTarget(check, ctx.cwd, subEnv) + if (!isOutsideWorkspace(ctx.contain, resolved)) return null + return { subCommand: sub, directory: check } +} + +function cdDirEscape( + dir: string, + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): CdEscape | null { + const word = norm.parenLocal ? dir.replace(/\)+$/, "") : dir + const poisoned = poisonedCdHit(word, sub, subEnv) + if (poisoned !== null) return poisoned + return containedCdHit(cdCheckWord(word, subEnv), sub, ctx, subEnv) +} + +function cdEscapeInArgv( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): CdEscape | null { + if (norm.opaqueEnv) return opaqueEnvCdEscape(norm, sub) + if (norm.argv0 !== "cd" && norm.argv0 !== "pushd") return null + const dir = cdDirOperand(norm.args) + if (norm.argv0 === "pushd" && isStackRotation(dir)) return null + if (dir === "-") return null + if (dir === null || dir === "") return homeEscape(sub, ctx, subEnv) + return cdDirEscape(dir, norm, sub, ctx, subEnv) +} + +function isStackRotation(dir: string | null): boolean { + return dir === null || /^[+-]\d+$/.test(dir) +} + +function trackCdDir( + norm: NormalizedArgv, + ctx: ScanCtx, + subEnv: ChainEnv, +): void { + const dir = cdDirOperand(norm.args) + if (dir === "-") { + const prev = ctx.prevCwd + ctx.prevCwd = ctx.cwd + ctx.cwd = prev + return + } + if (dir === null || dir === "") return + if (cdpathBlocks(dir, subEnv)) return + const word = norm.parenLocal ? dir.replace(/\)+$/, "") : dir + if (usesPoisonedVar(word, subEnv)) return + ctx.prevCwd = ctx.cwd + ctx.cwd = resolveOperandTarget(word === "" ? "~" : word, ctx.cwd, subEnv) +} + +function popDirStack(ctx: ScanCtx): void { + const top = ctx.stack.pop() + if (top === undefined) return + ctx.prevCwd = ctx.cwd + ctx.cwd = top +} + +function swapDirStack(ctx: ScanCtx): void { + const top = ctx.stack.pop() + if (top === undefined) return + ctx.stack.push(ctx.cwd) + ctx.prevCwd = ctx.cwd + ctx.cwd = top +} + +function pushdDirUntrackable(dir: string, subEnv: ChainEnv): boolean { + if (dir === "" || dir === "-") return true + if (isStackRotation(dir) || cdpathBlocks(dir, subEnv)) return true + return usesPoisonedVar(dir, subEnv) +} + +function trackPushdPopd( + norm: NormalizedArgv, + ctx: ScanCtx, + subEnv: ChainEnv, +): void { + if (norm.argv0 === "popd") { + popDirStack(ctx) + return + } + if (norm.argv0 !== "pushd") return + const dir = cdDirOperand(norm.args) + if (dir === null) { + swapDirStack(ctx) + return + } + if (pushdDirUntrackable(dir, subEnv)) return + ctx.stack.push(ctx.cwd) + ctx.prevCwd = ctx.cwd + ctx.cwd = resolveOperandTarget(dir, ctx.cwd, subEnv) +} + +/** Track inside-only directory changes for later sub-commands. Paren-local + * cds (subshells) never leak; rotations are no-ops (the stack holds only + * inside dirs, so order is irrelevant to containment). */ +function trackCd( + norm: NormalizedArgv | null, + ctx: ScanCtx, + subEnv: ChainEnv, +): void { + if (norm === null || norm.parenLocal || norm.opaqueEnv) return + if (norm.argv0 === "cd") { + trackCdDir(norm, ctx, subEnv) + return + } + trackPushdPopd(norm, ctx, subEnv) +} + +interface OperandRule { + values: string[] + scripts: string[] + skipValues: string[] + positionals: "last" | "all" | "script-rest" | "none" | "first" +} + +function lastDest(...values: string[]): OperandRule { + return { values, scripts: [], skipValues: [], positionals: "last" } +} + +function noPositionals(...values: string[]): OperandRule { + return { values, scripts: [], skipValues: [], positionals: "none" } +} + +function allPositionals(): OperandRule { + return { values: [], scripts: [], skipValues: [], positionals: "all" } +} + +function scriptRest(...skipValues: string[]): OperandRule { + const scripts = ["-e", "-f"] + return { values: [], scripts, skipValues, positionals: "script-rest" } +} + +const WRITE_OPERAND_RULES: Record = { + cp: lastDest("-t", "--target-directory"), + mv: lastDest("-t", "--target-directory"), + ln: lastDest("-t", "--target-directory"), + install: lastDest("-t", "--target-directory"), + rsync: lastDest(), + scp: lastDest(), + tee: allPositionals(), + git: noPositionals("-C", "--git-dir", "--work-tree"), + tar: noPositionals("-f", "--file", "-C", "--directory"), + sed: scriptRest(), + perl: scriptRest("-I", "-F"), + curl: noPositionals("-o", "--output", "--output-dir"), + wget: noPositionals("-O", "--output-document", "-P", "--directory-prefix"), + source: { values: [], scripts: [], skipValues: [], positionals: "first" }, + ".": { values: [], scripts: [], skipValues: [], positionals: "first" }, +} + +/** True when a single-dash cluster carries `letter` (`-pi` has `i`) or a + * long flag matches. Letters that consume the rest of the word stop the + * scan (`-ei` is `-e` with value `i`, not `-i`; `-tf` is `-t` with `f`). */ +function matchesLongFlag(word: string, longs: string[]): boolean { + return longs.some((long) => word === long || word.startsWith(`${long}=`)) +} + +function clusterCarries( + word: string, + letter: string, + stoppers: string[], +): boolean { + if (!/^-[^-]/.test(word)) return false + for (const char of word.slice(1)) { + if (stoppers.includes(`-${char}`)) return false + if (char === letter) return true + } + return false +} + +function clusterHasLetter( + args: string[], + letter: string, + stoppers: string[], + longs: string[], +): boolean { + for (const word of args) { + if (matchesLongFlag(word, longs)) return true + if (clusterCarries(word, letter, stoppers)) return true + } + return false +} + +/** Consume a value flag: separate (`-t DIR`), `--flag=DIR`, or attached + * (`-tDIR`) forms. Returns the next index to visit. */ +function attachedFlagValue(word: string, flags: string[]): string | null { + if (!/^-[A-Za-z]/.test(word) || word.length <= 2) return null + if (!flags.includes(word.slice(0, 2))) return null + return word.slice(2) +} + +function consumeValueFlag( + word: string, + args: string[], + index: number, + flags: string[], + into: string[], +): number { + if (flags.includes(word)) { + if (index + 1 < args.length) into.push(args[index + 1] ?? "") + return index + 1 + } + const attached = attachedFlagValue(word, flags) + if (attached !== null) { + into.push(attached) + return index + } + const eq = word.indexOf("=") + if (eq > 0 && flags.includes(word.slice(0, eq))) into.push(word.slice(eq + 1)) + return index +} + +/** tar's `-f` inside a cluster (`-cf ARCHIVE`, `-xffoo.tar`): the archive + * follows the `f` (same word) or the word. Separate from the table: `f` + * is positional-shape inside clusters, not a plain flag. */ +function tarClusterArchive(args: string[]): string | null { + for (let i = 0; i < args.length; i++) { + const word = args[i] ?? "" + if (!/^-[^-]/.test(word) || word === "-f") continue + const at = word.indexOf("f", 1) + if (at < 0) continue + if (at + 1 < word.length) return word.slice(at + 1) + return i + 1 < args.length ? (args[i + 1] ?? null) : null + } + return null +} + +interface CollectedOperands { + fromFlags: string[] + positional: string[] + scriptSeen: boolean +} + +interface FlagStep { + next: number + script: boolean +} + +function collectFlagWord( + word: string, + args: string[], + index: number, + rule: OperandRule, + fromFlags: string[], +): FlagStep { + const before = fromFlags.length + let next = consumeValueFlag(word, args, index, rule.values, fromFlags) + if (fromFlags.length === before) { + next = consumeValueFlag(word, args, next, rule.scripts, []) + } + const attached = attachedFlagValue(word, rule.scripts) !== null + const script = rule.scripts.includes(word) || attached + next = consumeValueFlag(word, args, next, rule.skipValues, []) + return { next: next + 1, script } +} + +interface CollectState extends CollectedOperands { + dashdash: boolean +} + +function collectNextArg( + args: string[], + index: number, + rule: OperandRule, + acc: CollectState, +): number { + const word = args[index] ?? "" + if (!acc.dashdash && word === "--") { + acc.dashdash = true + return index + 1 + } + if (!acc.dashdash && word.startsWith("-") && word.length > 1) { + const step = collectFlagWord(word, args, index, rule, acc.fromFlags) + acc.scriptSeen = acc.scriptSeen || step.script + return step.next + } + acc.positional.push(word) + return index + 1 +} + +function collectOperandCandidates( + args: string[], + rule: OperandRule, +): CollectedOperands { + const acc: CollectState = { + fromFlags: [], positional: [], scriptSeen: false, dashdash: false, + } + let i = 0 + while (i < args.length) i = collectNextArg(args, i, rule, acc) + const { fromFlags, positional, scriptSeen } = acc + return { fromFlags, positional, scriptSeen } +} + +function positionalCandidates( + collected: CollectedOperands, + rule: OperandRule, +): string[] { + const positional = collected.positional + if (rule.positionals === "last" && positional.length > 0) { + const last = positional[positional.length - 1] ?? "" + return [last] + } + if (rule.positionals === "all") return positional + if (rule.positionals === "first") return positional.slice(0, 1) + if (rule.positionals === "script-rest") { + return collected.scriptSeen ? positional : positional.slice(1) + } + return [] +} + +function gitRepoVarEscape( + name: string, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + const raw = subEnv.overlay.get(name) ?? process.env[name] + if (raw === undefined) return null + if (subEnv.poison.has(name) || usesPoisonedVar(raw, subEnv)) { + const reason = "unresolvable repo pointer" + return { subCommand: sub, command: "git", operand: name, reason } + } + const resolved = resolveOperandTarget(raw, ctx.cwd, subEnv) + if (!isOutsideWorkspace(ctx.contain, resolved)) return null + const operand = `${name}=${raw}` + return { subCommand: sub, command: "git", operand, reason: "git repo escapes" } +} + +function gitRepoEscape( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + if (norm.argv0 !== "git") return null + for (const name of ["GIT_DIR", "GIT_WORK_TREE"]) { + const hit = gitRepoVarEscape(name, sub, ctx, subEnv) + if (hit !== null) return hit + } + return null +} + +function wordWritesOutside( + word: string, + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + if (word === "" || (norm.argv0 === "scp" && word.includes(":"))) return null + const check = norm.parenLocal ? word.replace(/\)+$/, "") : word + if (usesPoisonedVar(check, subEnv)) { + const reason = "unresolvable variable" + return { subCommand: sub, command: norm.argv0, operand: word, reason } + } + const resolved = resolveOperandTarget(check, ctx.cwd, subEnv) + if (!isOutsideWorkspace(ctx.contain, resolved)) return null + return { subCommand: sub, command: norm.argv0, operand: word } +} + +function writesOutside( + words: string[], + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + for (const word of words) { + const hit = wordWritesOutside(word, norm, sub, ctx, subEnv) + if (hit !== null) return hit + } + return null +} + +function tarWriteHit( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + if (norm.argv0 !== "tar") return null + const absolute = clusterHasLetter(norm.args, "P", ["-f"], ["--absolute-names"]) + if (absolute) { + const reason = "absolute-name extraction" + return { subCommand: sub, command: "tar", operand: "-P", reason } + } + const archive = tarClusterArchive(norm.args) + if (archive === null) return null + return writesOutside([archive], norm, sub, ctx, subEnv) +} + +/** sed/perl without `-i` read their files (stdout carries the output); + * only in-place edits write, so only those are gated. */ +function inPlaceEditGated(norm: NormalizedArgv): boolean { + if (norm.argv0 !== "sed" && norm.argv0 !== "perl") return true + const base = ["-e", "-f"] + const stoppers = norm.argv0 === "perl" ? [...base, "-I", "-F"] : base + return clusterHasLetter(norm.args, "i", stoppers, ["--in-place"]) +} + +/** A hard link's TARGET is read (an outside inode smuggled in); a symlink + * target is inert text (file tools refuse to follow escapes later). */ +function hardlinkTargetHit( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, + positional: string[], +): WriteOperandEscape | null { + if (norm.argv0 !== "ln") return null + if (clusterHasLetter(norm.args, "s", ["-t"], ["--symbolic"])) return null + const first = positional[0] ?? "" + if (first === "") return null + return writesOutside([first], norm, sub, ctx, subEnv) +} + +function tableOperandEscape( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + const rule = WRITE_OPERAND_RULES[norm.argv0] + if (rule === undefined) return null + const tarHit = tarWriteHit(norm, sub, ctx, subEnv) + if (tarHit !== null) return tarHit + if (!inPlaceEditGated(norm)) return null + const collected = collectOperandCandidates(norm.args, rule) + const flagHit = writesOutside(collected.fromFlags, norm, sub, ctx, subEnv) + if (flagHit !== null) return flagHit + const positional = positionalCandidates(collected, rule) + const hit = writesOutside(positional, norm, sub, ctx, subEnv) + if (hit !== null) return hit + return hardlinkTargetHit(norm, sub, ctx, subEnv, collected.positional) +} + +const SHELL_COMMANDS = new Set(["sh", "bash", "dash", "zsh", "ksh"]) + +function shellOptionTakesValue(flag: string): boolean { + return flag === "-o" || flag === "-O" || flag === "+o" || flag === "+O" +} + +/** First script-file operand of a shell invocation (no `-c`): `-o`/`-O` + * values are consumed, every other dash-word skipped. */ +function shellScriptFile(args: string[]): string | null { + let i = 0 + while (i < args.length) { + const word = args[i] ?? "" + if (word === "--") return args[i + 1] ?? null + if (word.startsWith("-") && word.length > 1) { + i += shellOptionTakesValue(word) ? 2 : 1 + continue + } + return word + } + return null +} + +function findDashCScript(args: string[]): string | null { + for (let i = 0; i < args.length; i++) { + const word = args[i] ?? "" + if (word === "-c" || word === "--command") return args[i + 1] ?? null + if (word.startsWith("--command=")) return word.slice("--command=".length) + } + return null +} + +function shellStdinOpaque( + norm: NormalizedArgv, + sub: string, +): WriteOperandEscape { + const explicit = norm.args.some((word) => word === "-s" || word === "--stdin") + const operand = explicit ? "-s" : "(stdin)" + const reason = "stdin script is opaque" + return { subCommand: sub, command: norm.argv0, operand, reason } +} + +function shellScriptFileHit( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, +): WriteOperandEscape | null { + const file = shellScriptFile(norm.args) + if (file === null || file === "") return null + const resolved = resolveOperandTarget(file, ctx.cwd, subEnv) + if (!isOutsideWorkspace(ctx.contain, resolved)) return null + return { subCommand: sub, command: norm.argv0, operand: file } +} + +/** Shell invocations: `-c` scripts are scanned (forked context — a child + * shell never leaks exports/cwd); `-s`/redirected-stdin/bare shells are + * opaque (refused); a script FILE outside the workspace is refused (its + * contents are a documented boundary — inside scripts run unscanned). + * Piped-stdin shells (`... | bash`) are invisible per-sub-command and pass; + * flag-only probes (`bash --version`) are safe no-ops and pass. */ +function shellInvocationEscape( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, + out: ScanOut, +): WriteOperandEscape | null { + if (!SHELL_COMMANDS.has(norm.argv0)) return null + const script = findDashCScript(norm.args) + if (script !== null) { + scanPartsShared(parseCommand(script).commands, forkScanCtx(ctx), out) + return null + } + const dashS = norm.args.some((word) => word === "-s" || word === "--stdin") + if (dashS || norm.args.length === 0 || / 1) { + i += takesValue.has(word) ? 2 : 1 + continue + } + break + } + return args.slice(i) +} + +/** `read` binds names from input (unresolvable): poison its operands so + * later uses fail closed. */ +function poisonReadVars(norm: NormalizedArgv | null, env: ChainEnv): void { + if (norm === null || norm.argv0 !== "read") return + for (const name of readVarOperands(norm.args)) { + if (/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) { + env.poison.add(name) + env.overlay.delete(name) + } + } +} + +/** Atomic paren groups walk in a fork (subshells never leak cwd or + * exports); hits still propagate out. Returns whether it applied. */ +function scanParenGroup(sub: string, ctx: ScanCtx, out: ScanOut): boolean { + const trimmed = sub.trim() + if (!trimmed.startsWith("(")) return false + const inner = trimmed.replace(/^\(+/, "").replace(/\)+$/, "") + scanPartsShared(parseCommand(inner).commands, forkScanCtx(ctx), out) + return true +} + +/** Argv scans with explicit first-wins (never ??=): the op scan recurses + * into `-c`/eval scripts, recording hits as a side effect — a ??= would + * evaluate the nullish check BEFORE the RHS and clobber a + * recursion-recorded hit with the outer call's own null. */ +function scanSubArgv( + norm: NormalizedArgv, + sub: string, + ctx: ScanCtx, + subEnv: ChainEnv, + out: ScanOut, +): void { + const cdHit = cdEscapeInArgv(norm, sub, ctx, subEnv) + if (out.cd === null) out.cd = cdHit + const opHit = opEscapeInArgv(norm, sub, ctx, subEnv, out) + if (opHit !== null && out.op === null) out.op = opHit + poisonReadVars(norm, ctx.env) +} + +/** Check one sub-command (all three scans), then harvest its assignments + * and track its cd for LATER sub-commands. */ +function scanSubShared(sub: string, ctx: ScanCtx, out: ScanOut): void { + if (sub.trim() === "" || scanParenGroup(sub, ctx, out)) return + const words = splitCommandWords(sub) + const norm = normalizeArgv(words) + const subEnv = mergeChainEnv(ctx.env, norm?.local ?? newChainEnv()) + if (norm !== null) scanSubArgv(norm, sub, ctx, subEnv, out) + const redirHit = redirEscapeInSub(sub, ctx, subEnv) + if (out.redir === null) out.redir = redirHit + if (norm !== null) trackCd(norm, ctx, subEnv) + harvestSubAssignments(words, ctx.env) +} + +/** Ordered walk with resplit fixpoint: a sub-command that re-splits is + * promoted-subshell content (top-level operators always split), so its + * parts walk forked. Re-splitting strictly shrinks the strings. */ +function scanPartsShared(parts: string[], ctx: ScanCtx, out: ScanOut): void { + for (const sub of parts) { + const resplit = parseCommand(sub).commands + if (resplit.length > 1) { + scanPartsShared(resplit, forkScanCtx(ctx), out) + continue + } + scanSubShared(sub, ctx, out) + } +} + +function scanWholeCommand(command: string, workspaceRoot?: string): ScanOut { + const ctx = newScanCtx(workspaceRoot ?? process.cwd()) + prePoisonRawAssignments(command, ctx.env) + const out = newScanOut() + scanPartsShared(parseCommand(command).commands, ctx, out) + return out +} + +/** + * Check a command for a directory change escaping the workspace root: + * `cd`/`pushd` to an outside path, bare `cd` ($HOME), `cd` with an + * unresolvable operand, or an opaque environment (`env -i`). Per-sub-command + * argv analysis (unlike the redirect raw scan): `echo "cd /tmp"` and + * heredoc bodies naming cd are dead text, correctly allowed. + */ +export function checkCdEscape( + command: string, + workspaceRoot?: string, +): CdEscape | null { + return scanWholeCommand(command, workspaceRoot).cd +} + +/** + * Check a command for file-writing operands escaping the workspace root: + * copy/move/link/install/rsync/scp destinations, `tee` outputs, `git` + * repo pointers (`-C`, `--git-dir`, `--work-tree`, `GIT_DIR`), `tar` + * archives/dirs, in-place `sed`/`perl` files, downloader outputs, and + * shell script files. Reads (`cat /etc/x`) and remote scp (`h:/x`) pass. + */ +export function checkWriteOperandEscape( + command: string, + workspaceRoot?: string, +): WriteOperandEscape | null { + return scanWholeCommand(command, workspaceRoot).op +} + // ─── allow/deny logic port (zoo-code/src/core/auto-approval/commands.ts) ──── /** @@ -901,9 +1903,21 @@ export interface CommandRefusal { * - "protected_store": recursive delete targeting the shared central store * or a parent of it — always blocked, NOT configurable (see * src/permissions/store-protection.ts). + * - "cd_escape": a directory change escaping the workspace root — always + * blocked, NOT configurable (issue #136). + * - "write_escape": a file-writing operand escaping the workspace root — + * always blocked, NOT configurable (issue #136). */ - kind: "dangerous" | "malformed" | "redirect_escape" | "denied" | "not_allowed" | "protected_store" - /** The sub-command that triggered the refusal (denied/not_allowed/protected_store/redirect_escape). */ + kind: + | "dangerous" + | "malformed" + | "redirect_escape" + | "denied" + | "not_allowed" + | "protected_store" + | "cd_escape" + | "write_escape" + /** The sub-command that triggered the refusal (all kinds but dangerous). */ subCommand?: string /** The deny-list pattern that matched (denied). */ pattern?: string @@ -915,6 +1929,10 @@ export interface CommandRefusal { storeRoot?: string /** The redirect that escaped the workspace (redirect_escape). */ redirect?: RedirectTarget + /** The directory change that escaped the workspace (cd_escape). */ + cd?: CdEscape + /** The file-writing operand that escaped (write_escape). */ + writeOperand?: WriteOperandEscape } /** @@ -934,12 +1952,15 @@ export interface CommandRefusal { * to `/tmp`. Runs before parsing so it also catches redirects embedded in * unparseable fragments (e.g. a heredoc body's own `> /tmp` line). * 4. Each sub-command from `parseCommand` (so `echo hi && rm -rf /` is checked - * per sub-command, not as one opaque string): FIRST the central-store - * protection (recursive delete targeting the shared store or a parent of it - * — always refused, even with empty allow/deny config, and NOT overridable - * by any permissions config), THEN the redirect-escape guard per - * sub-command, then the deny-list, then the allow-list, with upstream's - * longest-prefix-match precedence. + * per sub-command, not as one opaque string), in one ordered walk that + * threads the cd-chain cwd and chain exports: FIRST the cd-escape guard + * (an outside-cd re-anchors every later relative path, so it refuses + * before anything else is resolved — issue #136), THEN the central-store + * protection (recursive delete targeting the shared store or a parent of + * it — always refused, even with empty allow/deny config, and NOT + * overridable by any permissions config), THEN the redirect-escape and + * write-operand guards per sub-command, then the deny-list, then the + * allow-list, with upstream's longest-prefix-match precedence. * * `options.workspaceRoot` anchors relative command targets for the * central-store and redirect-escape checks (the executor passes the command's @@ -960,9 +1981,15 @@ export function checkCommand( return { kind: "dangerous" } } - // Pre-parse redirect-escape scan: redirectTargets walks the raw command - // (heredoc bodies included), so it still catches redirects that parseCommand - // would classify as malformed (unterminated quote/heredoc). + // Pre-parse scans (cd first, then redirect): both walk the raw command + // (heredoc bodies included), so they still catch escapes that parseCommand + // would classify as malformed (unterminated quote/heredoc). cd precedes + // the redirect for the same reason it precedes it per sub-command: an + // outside-cd re-anchors every later relative path. + const cdEscape = checkCdEscape(command, options?.workspaceRoot) + if (cdEscape !== null) { + return { kind: "cd_escape", subCommand: command, cd: cdEscape } + } const redirectEscape = checkRedirectEscape(command, options?.workspaceRoot) if (redirectEscape !== null) { return { kind: "redirect_escape", subCommand: command, redirect: redirectEscape } @@ -973,72 +2000,63 @@ export function checkCommand( return { kind: "malformed", parseError, subCommand: command } } - // Track a leading `cd && ...` chain's effective cwd across - // sub-commands (SEC-6): `cd /tmp && rm -rf x` must resolve `x` against - // /tmp, not the pre-cd workspace root, or a relative-path rm evades the - // central-store check. Only a plain `cd ` sub-command updates the - // tracked cwd (best-effort — `cd` inside a subshell, via a variable, or - // via `pushd`/`popd` is not tracked; see store-protection.ts header). - let effectiveCwd = path.resolve(options?.workspaceRoot ?? process.cwd()) + // One ordered walk threads the cd-chain cwd, directory stack, and chain + // exports across sub-commands (SEC-6): relative targets resolve against + // the tracked cwd, containment always against the workspace root. + const ctx = newScanCtx(options?.workspaceRoot ?? process.cwd()) + prePoisonRawAssignments(command, ctx.env) for (const sub of subCommands) { const cmd = sub.replace(/\d*>&\d*/, "").trim() if (!cmd) { continue } - // Always-applied central-store protection: runs before (and - // independently of) the allow/deny lists, so the empty-allow-list - // default-ALLOW branch can never bypass it and no permissions.json - // entry can override it. - const storeRefusal = checkCentralStoreDestruction(cmd, effectiveCwd) - if (storeRefusal !== null) { - return { - kind: "protected_store", - subCommand: cmd, - target: storeRefusal.target, - storeRoot: storeRefusal.storeRoot, - } - } - // Per-sub-command redirect-escape guard (the pre-parse scan above is - // the same check on the whole command; this names the offending - // sub-command for the refusal message). Anchored to the tracked - // cd-chain cwd for the same reason as the store-destruction check - // above — a relative redirect after `cd /tmp && ...` must resolve - // against /tmp, not the pre-cd workspace root. - const subRedirect = checkRedirectEscape(cmd, effectiveCwd) - if (subRedirect !== null) { - return { kind: "redirect_escape", subCommand: cmd, redirect: subRedirect } - } - const refusal = checkSingleCommand(cmd, allowedCommands, deniedCommands) + const refusal = checkOneSubCommand(cmd, ctx, allowedCommands, deniedCommands) if (refusal !== null) { return refusal } - const cdTarget = matchLeadingCd(cmd) - if (cdTarget !== null) { - effectiveCwd = path.resolve(effectiveCwd, expandEnv(expandHome(cdTarget))) - } } return null } -/** - * When `cmd` is a plain `cd ` (optionally quoted, no other words), return - * the raw path argument; otherwise null. Deliberately narrow — `cd` combined - * with anything else on the same sub-command word (e.g. `cd /tmp; ls`, already - * split by parseCommand into separate sub-commands) or with no argument - * (`cd` alone, which goes to $HOME) is not tracked. - */ -function matchLeadingCd(cmd: string): string | null { - const m = cmd.match(/^cd\s+(.+)$/) - if (!m) { - return null +function checkSubStore(cmd: string, ctx: ScanCtx): CommandRefusal | null { + const storeRefusal = checkCentralStoreDestruction(cmd, ctx.cwd, ctx.env) + if (storeRefusal === null) return null + return { + kind: "protected_store", + subCommand: cmd, + target: storeRefusal.target, + storeRoot: storeRefusal.storeRoot, } - let arg = m[1].trim() - if ((arg.startsWith('"') && arg.endsWith('"')) || (arg.startsWith("'") && arg.endsWith("'"))) { - arg = arg.slice(1, -1) +} + +function checkSubScans(cmd: string, ctx: ScanCtx): CommandRefusal | null { + const out = newScanOut() + scanSubShared(cmd, ctx, out) + if (out.cd !== null) { + return { kind: "cd_escape", subCommand: cmd, cd: out.cd } } - return arg || null + const store = checkSubStore(cmd, ctx) + if (store !== null) return store + if (out.redir !== null) { + return { kind: "redirect_escape", subCommand: cmd, redirect: out.redir } + } + if (out.op !== null) { + return { kind: "write_escape", subCommand: cmd, writeOperand: out.op } + } + return null +} + +function checkOneSubCommand( + cmd: string, + ctx: ScanCtx, + allowedCommands: string[], + deniedCommands: string[], +): CommandRefusal | null { + const scans = checkSubScans(cmd, ctx) + if (scans !== null) return scans + return checkSingleCommand(cmd, allowedCommands, deniedCommands) } function checkSingleCommand( diff --git a/src/permissions/store-protection.ts b/src/permissions/store-protection.ts index df800d8..7da8651 100644 --- a/src/permissions/store-protection.ts +++ b/src/permissions/store-protection.ts @@ -185,18 +185,220 @@ export function expandEnv(p: string, env: NodeJS.ProcessEnv = process.env): stri .replace(/\$([A-Za-z_][A-Za-z0-9_]*)/g, (m, name: string) => env[name] ?? m) } +/** + * Chain-local variable bindings harvested from earlier sub-commands + * (`export NAME=value`, bare `NAME=value`): the runtime shell would expand + * them, so the gate must too — otherwise every `export OUT=dir && ... $OUT` + * chain is a false positive. `poison` holds names bound to unresolvable + * values (command substitutions, input, globs): uses of those fail closed. + */ +export interface ChainEnv { + overlay: Map + poison: Set +} + +export function newChainEnv(): ChainEnv { + return { overlay: new Map(), poison: new Set() } +} + +function isValidEnvName(name: string): boolean { + return /^[A-Za-z_][A-Za-z0-9_]*$/.test(name) +} + +function valueHasSpecials(value: string): boolean { + return /[$`\\*?[\]()<>|&;#\s]/.test(value) +} + +/** Record one NAME=value binding: static values overlay, dynamic values + * poison. An empty harvest never clears poison (it may be a mangled + * `NAME=$(...)` whose substitution parseCommand promoted away). */ +function harvestBinding(env: ChainEnv, name: string, value: string): void { + if (!isValidEnvName(name)) return + if (valueHasSpecials(value)) { + env.poison.add(name) + env.overlay.delete(name) + return + } + if (value !== "") env.poison.delete(name) + env.overlay.set(name, value) +} + +function splitAssignment(word: string): [string, string] | null { + const eq = word.indexOf("=") + if (eq <= 0) return null + const name = word.slice(0, eq) + if (!isValidEnvName(name)) return null + return [name, word.slice(eq + 1)] +} + +/** Harvest `export A=1 B=2` / bare `A=1 B=2` assignments from split words. + * `A=1 cmd` (env prefix + command) harvests NOTHING — prefixes are + * temporary, not bindings. Bare `export NAME` (no `=`) is a no-op. */ +export function harvestSubAssignments(words: string[], env: ChainEnv): void { + if (words[0] === "export") { + for (const word of words.slice(1)) { + const binding = splitAssignment(word) + if (binding !== null) harvestBinding(env, binding[0], binding[1]) + } + return + } + const bindings = words.map(splitAssignment) + if (bindings.some((binding) => binding === null)) return + for (const binding of bindings) { + if (binding !== null) harvestBinding(env, binding[0], binding[1]) + } +} + +/** Pre-poison names assigned from dynamic values in the RAW command. The + * ordered walk sees `NAME=` (empty) for `NAME=$(...)` — the substitution + * was promoted to its own sub-command — so without this pass a later + * static-looking harvest would clear poison that must stick. */ +export function prePoisonRawAssignments(command: string, env: ChainEnv): void { + const re = /(?:^|[;&|]|\s)(?:export\s+)?([A-Za-z_]\w*)=([^\s;&|]*)/g + let match: RegExpExecArray | null + while ((match = re.exec(command)) !== null) { + if (valueHasSpecials(match[2] ?? "")) { + env.poison.add(match[1] ?? "") + env.overlay.delete(match[1] ?? "") + } + } +} + +function isKnownVar(name: string, env: ChainEnv): boolean { + if (env.poison.has(name) || name === "") return false + if (env.overlay.has(name)) return true + return process.env[name] !== undefined +} + +function poisonedDefaultOp( + match: string, + op: string, + fallback: string, +): string { + if (op === ":+" || op === "+") return fallback + return match +} + +function knownDefaultOp( + name: string, + op: string, + fallback: string, + env: ChainEnv, +): string { + if (op === ":+" || op === "+") return fallback + return env.overlay.get(name) ?? process.env[name] ?? "" +} + +function unsetDefaultOp( + name: string, + op: string, + fallback: string, + env: ChainEnv, +): string { + if (op === ":=" || op === "=") { + harvestBinding(env, name, fallback) + return fallback + } + if (op === ":-" || op === "-") return fallback + return "" +} + +function defaultOpValue( + match: string, + name: string, + op: string, + fallback: string, + env: ChainEnv, +): string { + if (env.poison.has(name)) return poisonedDefaultOp(match, op, fallback) + if (isKnownVar(name, env)) return knownDefaultOp(name, op, fallback, env) + return unsetDefaultOp(name, op, fallback, env) +} + +/** Expand `${V:-D}` / `${V:=D}` / `${V:+A}` / `${V:?M}` (and bare `-`/`=`/ + * `+`/`?`) plus `${#V}`. `:=` on a truly-unset var assigns (as at + * runtime); on a poisoned var the match is left for the poison guard — + * `:=` never assigns over a set var, poisoned ones included. */ +function expandOperandDefaults(word: string, env: ChainEnv): string { + const pattern = /\$\{([A-Za-z_]\w*)(:-|:=|:\+|:\?|-|=|\+|\?)([^}]*)\}/g + const expand = (m: string, n: string, o: string, f: string): string => + defaultOpValue(m, n, o, f, env) + const withDefault = word.replace(pattern, expand) + return withDefault.replace(/\$\{#[A-Za-z_]\w*\}/g, "") +} + +/** Strip references to unknown/poisoned vars. Truly-unset vars expand to + * empty at runtime, so stripping is EXACT (not an approximation): `>$U/x` + * really writes `/x`, while bare `>$U` really errors (safe). Special vars + * ($$, $?, $1...) are shell-provided, never stripped. */ +function stripUnknownRefs(word: string, env: ChainEnv): string { + const keepKnown = (m: string, name: string): string => { + return isKnownVar(name, env) ? m : "" + } + const braced = word.replace(/\$\{([A-Za-z_]\w*)\}/g, keepKnown) + return braced.replace(/\$([A-Za-z_]\w*)/g, keepKnown) +} + +/** True when the word references a poisoned (unresolvable-valued) var. + * Unlike truly-unset vars (which expand to empty — stripped exactly), a + * poisoned var holds an UNKNOWN value, so any gated use fails closed. */ +export function usesPoisonedVar(word: string, env: ChainEnv): boolean { + const names: string[] = [] + word.replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)[^}]*\}/g, (_m, name: string) => { + names.push(name) + return _m + }) + word.replace(/\$([A-Za-z_][A-Za-z0-9_]*)/g, (_m, name: string) => { + names.push(name) + return _m + }) + return names.some((name) => env.poison.has(name)) +} + +/** The expanded-but-unresolved word: env + defaults + stripping, before + * `~` and resolve. Lets `cd` distinguish "expanded to empty" (→ $HOME, + * refused fail-closed) from literal `.` (stays, allowed). */ +export function expandOperandWord( + word: string, + env: ChainEnv = newChainEnv(), +): string { + const merged = { ...process.env } + for (const [name, value] of env.overlay) merged[name] = value + const expanded = expandEnv(expandOperandDefaults(word, env), merged) + return stripUnknownRefs(expanded, env) +} + +/** + * Resolve a raw operand word to an absolute path: chain-overlay + process + * env expansion (`${V:-D}` defaults honored, `:=` assigns), unknown-var + * stripping (exact — unset vars expand to empty), `~` expansion, then + * `path.resolve` against the command's working directory. TOTAL: unknown + * input degrades to a stripped path whose containment check fails closed. + */ +export function resolveOperandTarget( + word: string, + cwd: string, + env: ChainEnv = newChainEnv(), +): string { + return path.resolve(cwd, expandHome(expandOperandWord(word, env))) +} + /** * Resolve a raw target word to an absolute path: env expansion, `~` * expansion, then `path.resolve` against the command's working directory * (relative targets anchor there). A trailing `/*` glob is dropped — deleting * "the store's contents" via `rm -rf /*` targets the store itself. */ -export function resolveCommandTarget(target: string, workspaceRoot: string): string { - let expanded = expandEnv(expandHome(target)) +export function resolveCommandTarget( + target: string, + workspaceRoot: string, + env: ChainEnv = newChainEnv(), +): string { + let expanded = resolveOperandTarget(target, workspaceRoot, env) if (expanded.endsWith("/*")) { expanded = expanded.slice(0, -2) } - return path.resolve(workspaceRoot, expanded) + return expanded } /** @@ -256,17 +458,32 @@ export interface StoreDestructionRefusal { * store-targeting destructive command. `workspaceRoot` anchors relative * targets (defaults to process.cwd()). */ -export function checkCentralStoreDestruction(subCommand: string, workspaceRoot?: string): StoreDestructionRefusal | null { +export function checkCentralStoreDestruction( + subCommand: string, + workspaceRoot?: string, + env: ChainEnv = newChainEnv(), +): StoreDestructionRefusal | null { const words = splitCommandWords(subCommand) if (!isRecursiveDelete(words)) { return null } const root = path.resolve(workspaceRoot ?? process.cwd()) for (const target of deleteTargets(words)) { - const resolved = resolveCommandTarget(target, root) - if (isCentralStoreOrParent(resolved)) { - return { target: resolved, storeRoot: projectStoreRoot() } - } + const hit = storeTargetHit(target, root, env) + if (hit !== null) return hit } return null } + +function storeTargetHit( + target: string, + root: string, + env: ChainEnv, +): StoreDestructionRefusal | null { + if (usesPoisonedVar(target, env)) { + return { target, storeRoot: projectStoreRoot() } + } + const resolved = resolveCommandTarget(target, root, env) + if (!isCentralStoreOrParent(resolved)) return null + return { target: resolved, storeRoot: projectStoreRoot() } +} diff --git a/src/tools/__tests__/execute_command.test.ts b/src/tools/__tests__/execute_command.test.ts index 9ceb71f..ec40f9c 100644 --- a/src/tools/__tests__/execute_command.test.ts +++ b/src/tools/__tests__/execute_command.test.ts @@ -383,6 +383,51 @@ async function testOutsideRedirectRefusedBeforeSpawn(): Promise { } } +type HeadlessTestExecutor = ReturnType + +async function assertConfinedDenied( + executor: HeadlessTestExecutor, + cmd: string, + phrase: RegExp, +): Promise { + const result = await executor.execute("execute_command", { command: cmd, timeout: 10 }) + assert.equal(result.isError, true, `'${cmd}' must be refused`) + assert.match(result.content, /outside the workspace/) + assert.match(result.content, phrase) +} + +async function assertOutsideRefused( + executor: HeadlessTestExecutor, + marker: string, +): Promise { + const touchEscape = `cd /tmp && touch ${marker}` + await assertConfinedDenied(executor, touchEscape, /changes directory/) + await assertConfinedDenied(executor, `cd ..`, /changes directory/) + await assertConfinedDenied(executor, `cp a ${marker}`, /writes outside/) + await assertConfinedDenied(executor, `git -C /tmp status`, /writes outside/) + assert.equal(await exists(marker), false, "refused commands must never spawn") +} + +async function testOutsideCdAndOperandsRefusedBeforeSpawn(): Promise { + const ws = await mkTmpWorkspace("hc-ec-confine-") + const marker = path.join(os.tmpdir(), `hc-ec-confine-${process.pid}`) + await fs.rm(marker, { force: true }) + try { + await fs.mkdir(path.join(ws, "sub")) + const executor = createHeadlessExecutor(ws) + await assertOutsideRefused(executor, marker) + const inside = await executor.execute("execute_command", { + command: "cd sub && echo hello > out.txt && cp out.txt copy.txt", + timeout: 10, + }) + assert.equal(inside.isError, false, "inside cd/cp must run") + assert.equal(await exists(path.join(ws, "sub", "copy.txt")), true) + } finally { + await fs.rm(ws, { recursive: true, force: true }) + await fs.rm(marker, { force: true }) + } +} + /** * CI safety net for issue #35's fix: `BASH_PATH` must resolve to a REAL * bash binary on whatever machine runs this test, not silently fall back to @@ -604,6 +649,7 @@ const tests: Array<[string, () => Promise]> = [ ["execute_command: rm -rf of workspace scratch still runs (not the store)", testWorkspaceScratchCleanupStillAllowed], ["execute_command: non-destructive store access (echo) is allowed", testNonDestructiveStoreAccessAllowed], ["execute_command: outside-workspace redirect (/tmp, $VAR, compound) refused before spawn; inside redirect runs", testOutsideRedirectRefusedBeforeSpawn], + ["confined cd/operands gate", testOutsideCdAndOperandsRefusedBeforeSpawn], ["execute_command: bash-only syntax (${PIPESTATUS[0]}) does not falsely fail the command", testBashOnlySyntaxDoesNotFalselyFailTheCommand], ["execute_command: BASH_PATH resolves to a real binary (CI safety net for issue #35)", testBashPathResolvesToARealBinary], ["execute_command: oversized stdout bounded at the cap, pipe drains, truncation trailer present", testOversizedOutputIsBoundedAtCap], diff --git a/src/tools/executor.ts b/src/tools/executor.ts index 4c921a3..fb8467b 100644 --- a/src/tools/executor.ts +++ b/src/tools/executor.ts @@ -98,9 +98,12 @@ import { runTestsHandler } from "./run-tests.js" import type { AuxLlmUsage, ToolContext, ToolHandler, ToolResult } from "../engine/types.js" import { + checkCdEscape, checkCommand, checkRedirectEscape, + describeCdEscape, describeRedirect, + describeWriteEscape, type CommandRefusal, type RedirectTarget, } from "../permissions/commands.js" @@ -503,9 +506,53 @@ function refusalMessage(command: string, refusal: CommandRefusal): ToolResult { `Write scratch files under /.headlesscode/scratch/ instead.`, ) } + default: + return confinementRefusal(command, refusal) } } +function confinementRefusal( + command: string, + refusal: CommandRefusal, +): ToolResult { + if (refusal.kind === "cd_escape") { + return cdEscapeRefusal(command, refusal) + } + if (refusal.kind === "write_escape") { + return writeEscapeRefusal(command, refusal) + } + throw new Error(`unhandled refusal kind: ${refusal.kind}`) +} + +function cdEscapeRefusal(command: string, refusal: CommandRefusal): ToolResult { + const cd = refusal.cd + const shown = cd === undefined ? "a directory change" : describeCdEscape(cd) + return err( + `execute_command: refusing to run '${command}': it changes directory ` + + `outside the workspace root ('${shown}') — every later relative ` + + `path would resolve outside the workspace. Stay inside the ` + + `workspace (cd to a workspace-relative directory). This cannot ` + + `be overridden via --allowed-commands or permissions.json.`, + ) +} + +function writeEscapeRefusal( + command: string, + refusal: CommandRefusal, +): ToolResult { + const operand = refusal.writeOperand + const fallback = "a file write" + const shown = operand === undefined ? fallback : describeWriteEscape(operand) + return err( + `execute_command: refusing to run '${command}': it writes outside ` + + `the workspace root ('${shown}') — the resolved path is outside ` + + `the workspace and cannot be written from a harness session. ` + + `This is the same boundary every file tool enforces and cannot ` + + `be overridden via --allowed-commands or permissions.json. ` + + `Write inside the workspace instead.`, + ) +} + function truncate(content: string): string { if (content.length <= MAX_RESULT_CHARS) { return content @@ -1552,22 +1599,29 @@ function executeCommandHandler(args: Record, ctx: ToolContext, } // Permissions gate (command allow/deny + dangerous substitution + - // central-store protection + redirect-escape): refuse BEFORE spawning - // so nothing is ever executed. Compound commands are checked per - // sub-command (parseCommand splits on &&/||/;/|/&) — a denied or - // unallow-listed sub-command blocks the whole chain. This applies to - // every executor built on this handler, including the read-only - // reviewer/QA executors (they share it), which also run unattended and - // could otherwise run arbitrary commands. A refusal is a real tool - // error (isError: true) and therefore counts toward the loop's - // consecutive-mistake bound, same as any tool error. The command's - // resolved `cwd` anchors relative targets for the central-store and - // redirect-escape checks (see src/permissions/store-protection.ts and + // central-store protection + redirect/cd/write-operand escapes): + // refuse BEFORE spawning so nothing is ever executed. Compound + // commands are checked per sub-command (parseCommand splits on + // &&/||/;/|/&) — a denied or unallow-listed sub-command blocks the + // whole chain. This applies to every executor built on this handler, + // including the read-only reviewer/QA executors (they share it), + // which also run unattended and could otherwise run arbitrary + // commands. A refusal is a real tool error (isError: true) and + // therefore counts toward the loop's consecutive-mistake bound, same + // as any tool error. The command's resolved `cwd` anchors relative + // targets for the central-store, redirect, cd, and write-operand + // checks (see src/permissions/store-protection.ts and // src/permissions/commands.ts). The redirect-escape scan runs against // the RAW command before checkCommand parses it (a redirect embedded // in an unparseable fragment — e.g. a heredoc body's `> /tmp` line — // would otherwise be swallowed by the malformed-command path). const redirectRoot = cwd + const cdEscape = checkCdEscape(command, redirectRoot) + if (cdEscape !== null) { + return refusalMessage(command, { + kind: "cd_escape", subCommand: command, cd: cdEscape, + }) + } const redirectEscape = checkRedirectEscape(command, redirectRoot) if (redirectEscape !== null) { return refusalMessage(command, { kind: "redirect_escape", subCommand: command, redirect: redirectEscape })