From d0334ca54466e87918b641fa5f4ae185c1fb10bb Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Fri, 29 May 2026 22:22:51 +0000 Subject: [PATCH 1/2] fix(coder): separator-bounded worktree path guard in acp-client-fs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP fs bridge's worktree guard used an unbounded `startsWith(resolve( worktreePath))`, so a sibling path sharing the worktree as a string prefix (`-evil/...`) escaped the scope. Since writeWorktreeTextFile hits disk directly (no pending_changes gate), a confused/buggy ACP agent could write outside its worktree. Now uses a separator-bounded check matching write_guard.ts (resolve() + `startsWith(root + sep)` / `=== root`) via a shared resolveInWorktree, with a regression test (../ traversal + the sibling-prefix bug). Symlink-swap hardening intentionally skipped — consistent with write_guard's no-realpath stance; the agent runs with host FS access so this is a containment guard, not a trust boundary. Flagged by the automated push security review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../services/__tests__/acp-client-fs.test.ts | 50 +++++++++++++++++++ apps/coder/src/services/acp-client-fs.ts | 32 ++++++++---- 2 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 apps/coder/src/services/__tests__/acp-client-fs.test.ts diff --git a/apps/coder/src/services/__tests__/acp-client-fs.test.ts b/apps/coder/src/services/__tests__/acp-client-fs.test.ts new file mode 100644 index 0000000..fe782e4 --- /dev/null +++ b/apps/coder/src/services/__tests__/acp-client-fs.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { readWorktreeTextFile, writeWorktreeTextFile } from '../acp-client-fs.js'; + +const created: string[] = []; +function freshWorktree(): string { + const wt = mkdtempSync(join(tmpdir(), 'acp-wt-')); + created.push(wt); + return wt; +} +afterEach(() => { + for (const d of created.splice(0)) { + try { + rmSync(d, { recursive: true, force: true }); + rmSync(`${d}-evil`, { recursive: true, force: true }); + } catch { + /* ignore */ + } + } +}); + +describe('acp-client-fs worktree scoping', () => { + it('writes then reads a file inside the worktree', async () => { + const wt = freshWorktree(); + await writeWorktreeTextFile(wt, 'sub/dir/note.txt', 'hello'); + expect(await readWorktreeTextFile(wt, 'sub/dir/note.txt')).toBe('hello'); + }); + + it('rejects ../ traversal on read', async () => { + const wt = freshWorktree(); + await expect(readWorktreeTextFile(wt, '../../etc/passwd')).rejects.toThrow(/escapes worktree/); + }); + + it('rejects ../ traversal on write', async () => { + const wt = freshWorktree(); + await expect(writeWorktreeTextFile(wt, '../escape.txt', 'x')).rejects.toThrow(/escapes worktree/); + }); + + it('rejects a sibling-prefix path (the unbounded-startsWith bug)', async () => { + const wt = freshWorktree(); + // Absolute path that shares the worktree as a STRING prefix but is a sibling + // dir: `-evil/...`. A bare `startsWith()` wrongly admits it. + await expect(readWorktreeTextFile(wt, `${wt}-evil/secret.txt`)).rejects.toThrow(/escapes worktree/); + await expect(writeWorktreeTextFile(wt, `${wt}-evil/secret.txt`, 'x')).rejects.toThrow( + /escapes worktree/, + ); + }); +}); diff --git a/apps/coder/src/services/acp-client-fs.ts b/apps/coder/src/services/acp-client-fs.ts index 87d6061..e2a0d04 100644 --- a/apps/coder/src/services/acp-client-fs.ts +++ b/apps/coder/src/services/acp-client-fs.ts @@ -1,5 +1,25 @@ import { promises as fs } from 'node:fs'; -import { dirname, isAbsolute, join, resolve } from 'node:path'; +import { dirname, isAbsolute, resolve, sep } from 'node:path'; + +/** + * Resolve an ACP-supplied path against the agent worktree and reject anything + * that escapes it. Mirrors `write_guard.ts`'s check: `resolve()` to normalize + * `../` segments, then a **separator-bounded** prefix test — a bare + * `startsWith(root)` wrongly admits a sibling dir like `-evil/...`. + * + * No realpath (consistent with `write_guard.ts`: the target may not exist yet on + * write). This is a containment guard for the ACP fs bridge, not a hard trust + * boundary — the agent process already runs with host FS access; symlink-swap + * hardening (`O_NOFOLLOW`/realpath) is out of scope here. + */ +function resolveInWorktree(worktreePath: string, filePath: string): string { + const root = resolve(worktreePath); + const absolute = isAbsolute(filePath) ? resolve(filePath) : resolve(root, filePath); + if (absolute !== root && !absolute.startsWith(root + sep)) { + throw new Error(`path escapes worktree: ${filePath}`); + } + return absolute; +} /** Resolve an ACP path against the agent worktree and read a slice of lines. */ export async function readWorktreeTextFile( @@ -8,10 +28,7 @@ export async function readWorktreeTextFile( line?: number | null, limit?: number | null, ): Promise { - const absolute = isAbsolute(filePath) ? filePath : resolve(worktreePath, filePath); - if (!absolute.startsWith(resolve(worktreePath))) { - throw new Error(`path escapes worktree: ${filePath}`); - } + const absolute = resolveInWorktree(worktreePath, filePath); const raw = await fs.readFile(absolute, 'utf8'); if (!line && !limit) return raw; const lines = raw.split(/\r?\n/); @@ -26,10 +43,7 @@ export async function writeWorktreeTextFile( filePath: string, content: string, ): Promise { - const absolute = isAbsolute(filePath) ? filePath : resolve(worktreePath, filePath); - if (!absolute.startsWith(resolve(worktreePath))) { - throw new Error(`path escapes worktree: ${filePath}`); - } + const absolute = resolveInWorktree(worktreePath, filePath); await fs.mkdir(dirname(absolute), { recursive: true }); await fs.writeFile(absolute, content, 'utf8'); } From 63adb218e6bca4d15eb982925f7d8be6144659ba Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Fri, 29 May 2026 22:23:13 +0000 Subject: [PATCH 2/2] chore(coder): untrack live coder-providers.json, ship example The live config is read AND written by the coder (UI provider toggles PATCH it), so tracking it churned `git status`. Untrack it (now gitignored under data/*), add a tracked data/coder-providers.example.json reference, and update the .gitignore exception + CLAUDE.md/BOOCODER.md docs. Loader already falls back to {providers:{}} (built-ins only) when the live file is absent. + CHANGELOG v2.5.15. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitignore | 2 +- BOOCODER.md | 2 +- CHANGELOG.md | 4 ++++ CLAUDE.md | 2 +- data/coder-providers.example.json | 12 ++++++++++++ data/coder-providers.json | 3 --- 6 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 data/coder-providers.example.json delete mode 100644 data/coder-providers.json diff --git a/.gitignore b/.gitignore index 5f8ebd3..bb3c3ba 100644 --- a/.gitignore +++ b/.gitignore @@ -16,5 +16,5 @@ data/* !data/AGENTS.md !data/skills/ !data/mcp.json -!data/coder-providers.json +!data/coder-providers.example.json codecontext/fork.tar.gz diff --git a/BOOCODER.md b/BOOCODER.md index 960e094..8e87d85 100644 --- a/BOOCODER.md +++ b/BOOCODER.md @@ -44,7 +44,7 @@ BooCoder's coding agents are a **config-backed registry**: built-ins live in `pr ### Config file: `data/coder-providers.json` -Resolved from `CODER_PROVIDERS_PATH` (default `/data/coder-providers.json`; dev/host path `/opt/boocode/data/coder-providers.json`). It is **tracked in git** via a `.gitignore` exception (the rest of `data/*` is ignored). A missing file, invalid JSON, or a schema mismatch all fall back to built-ins-only — loading never throws at startup. +Resolved from `CODER_PROVIDERS_PATH` (default `/data/coder-providers.json`; dev/host path `/opt/boocode/data/coder-providers.json`). It is **gitignored** — it's live runtime config that the coder reads *and writes* (UI toggles `PATCH` it), so tracking it would churn `git status`. The tracked reference is `data/coder-providers.example.json`; copy it to `coder-providers.json` to seed overrides. A missing file, invalid JSON, or a schema mismatch all fall back to built-ins-only — loading never throws at startup. ```json { diff --git a/CHANGELOG.md b/CHANGELOG.md index 62c0508..02af5fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ All notable changes per release tag. Most recent on top, ordered by tag creation date (which matches the git history). Tag names follow `vMAJOR.MINOR.PATCH-slug` — the slug describes what shipped, so the tag name alone is enough to recall the batch. +## v2.5.15-acp-path-guard — 2026-05-29 + +Security fix + repo hygiene. Fixes a path-traversal in the ACP filesystem bridge (`acp-client-fs.ts`, flagged by the automated push security review): the worktree guard used an unbounded `startsWith(resolve(worktreePath))`, so a sibling path sharing the worktree as a string prefix (`-evil/…`) escaped the scope — and `writeWorktreeTextFile` writes to disk directly (no `pending_changes` gate), so a confused/buggy ACP agent could write outside its worktree. Now uses a separator-bounded check matching `write_guard.ts` (`resolve()` + `startsWith(root + sep)` / `=== root`) via a shared `resolveInWorktree`, with a regression test covering `../` traversal and the sibling-prefix bug. Symlink-swap/`O_NOFOLLOW` hardening was intentionally skipped — consistent with `write_guard`'s no-realpath stance, and the agent already runs with host FS access so this is a containment guard, not a trust boundary. Separately, stops tracking the live `data/coder-providers.json` (it's runtime config the UI reads *and writes* on provider toggles, which churned `git status`) — it's now gitignored with a tracked `data/coder-providers.example.json` reference; the loader falls back to built-ins-only when the live file is absent. The provider-type duplication (coder ↔ web) stays guarded by the existing text-identity `provider-types-parity.test.ts` — a shared package was considered and declined (drift is already prevented; not worth the Docker/build-order risk at solo scale). + ## v2.5.14-claude-md — 2026-05-29 Docs-only — CLAUDE.md session-learnings update, no code. Adds gotchas surfaced while shipping the v2.3 provider-lifecycle batch: the host `boocoder.service` keeps running the old process after `pnpm -C apps/coder build` (stale-process tell = new routes 404 while old routes 200, restart don't re-debug); the `boocode` container `build: .` deploys the working tree, so web edits are live on the Vite dev server but not production until `docker compose up --build -d boocode`; `PATCH /api/providers/config` replaces a provider's override wholesale (send `{...existing, enabled}` or a custom ACP entry's command is wiped) and `data/coder-providers.json` is live config not to be committed as code; external agents dispatch one-shot with no context/token tracking (only native `boocode` tracks ctx; OpenCode-as-server is the unshipped `v2-6-persistent-agent-sessions` plan); the `ui/` primitive inventory with `button role=switch` / Dialog fallbacks for the absent switch/sheet; and the mobile Dialog-with-list scroll-containment recipe. Also backfills previously-uncommitted doc bullets for the `v2.5.7`–`v2.5.11` coder work (provider-type parity test, async ACP command discovery, AgentComposerBar `installed` filter, provider-registry path disambiguation). diff --git a/CLAUDE.md b/CLAUDE.md index 9225eee..4df7497 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,7 +88,7 @@ Route registration: all routes registered in `index.ts` via `register*Routes(app - Frontend: NOT a separate SPA. BooCoder is a `'coder'` pane type within BooChat's SPA (`apps/web/`). `CoderPane.tsx` in `apps/web/src/components/panes/`. API requests go through `/api/coder/*` proxy (Vite dev + Fastify production) which rewrites to the boocoder host service (`BOOCODER_URL` env var, default `http://100.114.205.53:9502`). WS connects directly to `:9502`. - `apps/coder/web/` is a STANDALONE fallback SPA served at `:9502` directly. The PRIMARY BooCoder frontend is the `CoderPane` in BooChat's SPA (`apps/web/src/components/panes/CoderPane.tsx`), accessible via the "Coder" pane in the workspace at `code.indifferentketchup.com`. Both exist; the pane is what Sam uses. - **Provider snapshot lifecycle** (`apps/coder/src/services/`): `provider-config.ts` (Zod config, never-throws on bad input) → `provider-config-registry.ts` (`buildResolvedRegistry`, singleton) → `provider-snapshot.ts` (two-tier probe: tier-1 fast presence, tier-2 cold ACP probe skipped unless force / stale `PROVIDER_PROBE_TTL_MS` 24h / dbEmpty; cached). Verify live: `curl http://100.114.205.53:9502/api/providers/snapshot` — returns providers + models + commands, the exact shape `AgentComposerBar` renders. -- `PATCH /api/providers/config` replaces a provider id's override object **wholesale** (per-id shallow merge) — to flip one field send `{...existing, enabled}`, or a custom ACP entry's `command`/`label` is wiped and it drops out of the resolved registry. `data/coder-providers.json` is the live config (tracked via a `.gitignore` exception, bind-mounted); UI toggles mutate it on disk → working-tree drift, don't commit it as a code change. +- `PATCH /api/providers/config` replaces a provider id's override object **wholesale** (per-id shallow merge) — to flip one field send `{...existing, enabled}`, or a custom ACP entry's `command`/`label` is wiped and it drops out of the resolved registry. `data/coder-providers.json` is **gitignored** (it's live runtime config — the coder reads AND writes it on UI toggles); the tracked reference is `data/coder-providers.example.json`. The loader falls back to `{providers:{}}` (built-ins only) when the live file is absent, so a fresh checkout needs no copy. - External agents dispatch **one-shot** (`opencode acp` / `goose acp` / `qwen --acp`) and report no context-window/token usage; only native `boocode` (llama-swap engine) tracks ctx. OpenCode-as-HTTP-server (warm process + `@opencode-ai/sdk`, the source of a real context bar) is the **planned, unshipped** `openspec/changes/v2-6-persistent-agent-sessions` batch; Paseo's per-provider native clients (design §12) were deliberately not ported. ### Frontend (`apps/web/src/`) diff --git a/data/coder-providers.example.json b/data/coder-providers.example.json new file mode 100644 index 0000000..8295c9e --- /dev/null +++ b/data/coder-providers.example.json @@ -0,0 +1,12 @@ +{ + "providers": { + "goose": { "enabled": false }, + "amp-acp": { + "extends": "acp", + "label": "Amp", + "description": "ACP wrapper for Amp", + "command": ["amp-acp"], + "enabled": true + } + } +} diff --git a/data/coder-providers.json b/data/coder-providers.json deleted file mode 100644 index 87b07bc..0000000 --- a/data/coder-providers.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "providers": {} -}