diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eaa4d8..b2764c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.11.4] - 2026-08-22 + +### Fixed + +- Session file roots are backend-authoritative: `session/load`·`resume` no + longer adopt the client's cwd as the session root (a remote App derived + its cwd from the hub instance list and fell back to "/" whenever that list + was stale, hijacking the /fs file browser to the filesystem root and — via + `projectCwd()` — poisoning the instance's advertised workspace for every + later load). The root now comes from what the bridge recorded at creation, + corrected by the backend's own `session/resume` result + (`session.workspace.workspacePath`); a client cwd is only consulted at + `session/new`, and "/" is never accepted as a root anywhere. +- `/fs` defense in depth: a session whose recorded root resolves to "/" is + refused (403) — a polluted record can never widen remote file access to + the whole filesystem. +- `projectCwd()` picks the most recently active session's cwd (insertion + order was arbitrary across load timing) and skips "/" entries entirely. +- Tests: `tests/session-cwd.test.ts` (client-cwd trust boundary, backend + workspace adoption, polluted-entry healing) + a /fs root-"/" refusal case + in `tests/remote-file-endpoint.test.ts`. + ## [0.11.3] - 2026-08-22 ### Fixed diff --git a/package.json b/package.json index e320ab0..579b7d3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "zcode-acp-server", - "version": "0.11.3", + "version": "0.11.4", "description": "Agent Client Protocol (ACP) server bridging headless ZCode to editors like Zed and JetBrains.", "type": "module", "license": "Apache-2.0", diff --git a/src/handlers/session.ts b/src/handlers/session.ts index 7381420..ac7d919 100644 --- a/src/handlers/session.ts +++ b/src/handlers/session.ts @@ -49,6 +49,41 @@ function workspaceFor(cwd?: string): { workspacePath: string; workspaceKey: stri return { workspacePath: p, workspaceKey: p }; } +/** + * A client-supplied cwd is only ever trusted for `session/new` — creating a + * session is the editor declaring its worktree. "/" is never a project root: + * remote clients fall back to it when their instance list is stale, and a + * session root decides what the /fs file endpoint exposes. + */ +function sanitizeClientCwd(client: string | undefined): string | null { + return client && client !== "/" ? client : null; +} + +/** + * The authoritative Session Root for an EXISTING session: what the bridge + * already recorded (set at creation, or refreshed from the backend's resume + * result). Client cwds are NOT consulted — a remote client must not be able + * to widen or move a session's file scope by sending its own cwd. A + * previously-polluted "/" entry counts as unknown so the next resume + * repopulates it from the backend. + */ +function authoritativeSessionCwd(server: ZcodeAcpServer, acpSid: string): string { + const existing = server.sessionCwds.get(acpSid); + return existing && existing !== "/" ? existing : process.cwd(); +} + +/** + * Extract the backend-recorded workspace from a session/resume result + * (`result.session.workspace.workspacePath`). This is the session's own + * project directory as the backend sees it — the value remote file access + * is scoped to. Returns null when absent or malformed. + */ +function workspaceFromResumeResult(result: unknown): string | null { + const ws = (result as { session?: { workspace?: { workspacePath?: unknown } } } | null)?.session + ?.workspace?.workspacePath; + return typeof ws === "string" && ws !== "" && ws !== "/" ? ws : null; +} + /** * Push the provider registry to the backend so third-party providers (those in * config.json) are recognised. The V4 backend doesn't auto-load them from @@ -93,7 +128,9 @@ export async function newSession( server: ZcodeAcpServer, params: acp.NewSessionRequest, ): Promise { - const cwd = params.cwd ?? process.cwd(); + // Creation is the one moment a client's cwd is trusted (the editor + // declaring its worktree); "/" is still rejected as a degenerate root. + const cwd = sanitizeClientCwd(params.cwd) ?? process.cwd(); // Placeholder id — the client addresses this session with it until the // backend session materializes; never shown in session/list. const acpSid = randomUUID(); @@ -167,7 +204,7 @@ export async function ensureRealSession(server: ZcodeAcpServer, acpSid: string): if (record) { pending = { cwd: record.cwd }; server.pendingSessions.set(acpSid, pending); - server.sessionCwds.set(acpSid, record.cwd); + if (record.cwd !== "/") server.sessionCwds.set(acpSid, record.cwd); } } if (!pending) throw new Error(`session ${acpSid} not found`); @@ -347,8 +384,12 @@ export async function resumeSession( cx: acp.AgentContext, ): Promise { const acpSid = params.sessionId; - const cwd = params.cwd ?? process.cwd(); if (!acpSid) throw new Error("sessionId required"); + // The Session Root never comes from the client (params.cwd is ignored): + // start from what the bridge recorded, then let the backend's own resume + // result correct it below — a remote client must not move a session's + // file scope by sending its own cwd. + let cwd = authoritativeSessionCwd(server, acpSid); // Lazy placeholders (session/new) resolve to their real backend session // here; alreadyLive targets skip the resume RPC because the session is live @@ -377,14 +418,18 @@ export async function resumeSession( // third-party model in its history, and the backend needs the provider // registered to even process the resume turn. await syncProviderRegistry(server, cwd); - await resumeBackendSession(server, zcParams); + const resumeResult = await resumeBackendSession(server, zcParams); // The resume RPC succeeded — the session is now loaded in this backend. server.markBackendLoaded(acpSid); + // The backend's session record is the root authority: adopt its + // workspace as the session root (heals any stale/polluted entry). + const backendWs = workspaceFromResumeResult(resumeResult); + if (backendWs) cwd = backendWs; } server.registerSession(acpSid, zcodeSid); - // The load's cwd becomes the session root for remote file access (same as - // session/new) — without this, a loaded session has no readable root. + // The session root for remote file access — backend-authoritative (see + // above); without this, a loaded session has no readable root. server.sessionCwds.set(acpSid, cwd); log(`session/resume -> ${zcodeSid}`); server.ensureBackgroundListener(zcodeSid); @@ -410,8 +455,12 @@ export async function loadSession( cx: acp.AgentContext, ): Promise { const acpSid = params.sessionId; - const cwd = params.cwd ?? process.cwd(); if (!acpSid) throw new Error("sessionId required"); + // The Session Root never comes from the client (params.cwd is ignored): + // start from what the bridge recorded, then let the backend's own resume + // result correct it below — a remote client must not move a session's + // file scope by sending its own cwd. + let cwd = authoritativeSessionCwd(server, acpSid); // Same placeholder resolution as resumeSession; alreadyLive targets skip the // backend resume RPC (the session is live in this subprocess). @@ -428,12 +477,16 @@ export async function loadSession( // third-party model in its history, and the backend needs the provider // registered to process it. await syncProviderRegistry(server, cwd); - await resumeBackendSession(server, zcParams); + const resumeResult = await resumeBackendSession(server, zcParams); // The resume RPC succeeded — the session is now loaded in this backend. server.markBackendLoaded(acpSid); + // The backend's session record is the root authority: adopt its + // workspace as the session root (heals any stale/polluted entry). + const backendWs = workspaceFromResumeResult(resumeResult); + if (backendWs) cwd = backendWs; } server.registerSession(acpSid, zcodeSid); - // Same as resumeSession: record the cwd as the session root for file access. + // Same as resumeSession: backend-authoritative session root for file access. server.sessionCwds.set(acpSid, cwd); log(`session/load → ${zcodeSid}`); server.ensureBackgroundListener(zcodeSid); @@ -1166,11 +1219,14 @@ function fileUriToPath(uri: string): string { * can land in that gap and time out without the backend ever seeing it. A single * retry — issued after the startup window has elapsed — succeeds. Non-timeout * errors (Invalid params, session not found) fail fast. + * + * Returns the response's result object on success — callers extract the + * backend-authoritative session workspace from it. Throws on failure. */ async function resumeBackendSession( server: ZcodeAcpServer, zcParams: Record, -): Promise { +): Promise> { const backend = server.ensureBackend(); const MAX_ATTEMPTS = 2; const ATTEMPT_TIMEOUT_MS = 15_000; @@ -1181,7 +1237,7 @@ async function resumeBackendSession( zcParams, ATTEMPT_TIMEOUT_MS, ); - if (!resp.error) return; + if (!resp.error) return (resp.result ?? {}) as Record; const isTimeout = resp.error.message === "timeout"; if (!isTimeout || attempt === MAX_ATTEMPTS) { throw new Error(`zcode resume failed: ${resp.error.message ?? ""}`); @@ -1191,6 +1247,7 @@ async function resumeBackendSession( ); await sleep(1000); } + throw new Error("zcode resume failed: exhausted retries"); } /** diff --git a/src/remote/file-endpoint.ts b/src/remote/file-endpoint.ts index 36f25d0..d3dec2d 100644 --- a/src/remote/file-endpoint.ts +++ b/src/remote/file-endpoint.ts @@ -124,8 +124,20 @@ async function sessionRoot( sendText(res, 403, "unknown session"); return null; } + // Defense in depth: a session root of "/" is never legitimate (projects + // live in subdirectories; "/" could only come from a polluted cwd record). + // Serving it would expose the whole filesystem to remote clients. + if (cwd === "/") { + sendText(res, 403, "session root unavailable"); + return null; + } try { - return await realpath(cwd); + const real = await realpath(cwd); + if (real === "/") { + sendText(res, 403, "session root unavailable"); + return null; + } + return real; } catch { sendText(res, 404, "session root unavailable"); return null; diff --git a/src/server.ts b/src/server.ts index 25ef301..789dfe4 100644 --- a/src/server.ts +++ b/src/server.ts @@ -264,11 +264,26 @@ export class ZcodeAcpServer { } /** - * The bridge's project directory: first known session cwd, else the bridge - * process cwd (Zed spawns the server with the worktree root as cwd). + * The bridge's project directory: the cwd of the most recently active + * session, else the bridge process cwd (Zed spawns the server with the + * worktree root as cwd). Recent-activity wins over Map order — insertion + * order is arbitrary across load/resume timing, and a single polluted + * entry ("/") must never decide the label for every session. Roots of "/" + * are skipped entirely: they can only come from a client fallback, never a + * real worktree. */ projectCwd(): string { - return this.sessionCwds.values().next().value ?? process.cwd(); + let best = ""; + let bestAt = -1; + for (const [acpSid, cwd] of this.sessionCwds) { + if (!cwd || cwd === "/") continue; + const at = this.sessionSummaries.get(acpSid)?.updatedAt ?? 0; + if (at > bestAt) { + best = cwd; + bestAt = at; + } + } + return best || process.cwd(); } /** Best-effort workspace label for the hub discovery payload. */ diff --git a/tests/remote-file-endpoint.test.ts b/tests/remote-file-endpoint.test.ts index f14ba21..136868a 100644 --- a/tests/remote-file-endpoint.test.ts +++ b/tests/remote-file-endpoint.test.ts @@ -43,6 +43,8 @@ interface Fixture { /** Sibling dir outside the session root (for escape fixtures). */ outside: string; endpoint: { port: number; stop(): Promise }; + /** The bridge's server state (for polluting cwd records in tests). */ + server: ZcodeAcpServer; } async function spawnFixture(): Promise { @@ -82,6 +84,7 @@ async function spawnFixture(): Promise { dir, outside, endpoint: endpoint!, + server, }; } @@ -121,6 +124,17 @@ describe("session files over the hub proxy", () => { expect(bad.status).toBe(404); }); + it("refuses to serve a session whose recorded root is /", async () => { + const { base, server } = await spawnFixture(); + // A polluted cwd record must never widen file access to the filesystem + // root — defense in depth behind the load-side guards. + server.sessionCwds.set("s-polluted", "/"); + const res = await fsFetch(base, "/list?sessionId=s-polluted"); + expect(res.status).toBe(403); + const file = await fsFetch(base, "/file?sessionId=s-polluted&path=etc/passwd"); + expect(file.status).toBe(403); + }); + it("streams whole files with a Content-Type from the extension", async () => { const { base } = await spawnFixture(); const res = await fsFetch(base, "/file?sessionId=s-fs&path=README.md"); diff --git a/tests/session-cwd.test.ts b/tests/session-cwd.test.ts new file mode 100644 index 0000000..12c554a --- /dev/null +++ b/tests/session-cwd.test.ts @@ -0,0 +1,185 @@ +/** + * Tests for session-root (cwd) trust boundaries. + * + * The session root decides what the /fs file endpoint exposes to remote + * clients, so it must be backend-authoritative: a client's cwd is only + * consulted at session/new (the editor declaring its worktree), never for + * load/resume. The remote App used to send "/" as its cwd whenever its + * instance list was stale (a session switch races the 4s list poll) — a + * bridge that adopted it hijacked the file roots to the filesystem root and + * polluted the advertised workspace label. + */ + +import type * as acp from "@agentclientprotocol/sdk"; +import { describe, expect, it, vi } from "vitest"; + +import type { ZcodeBackend } from "../src/backend/client.js"; +import type { ZcodeMessage } from "../src/backend/types.js"; +import { loadSession, newSession } from "../src/handlers/session.js"; +import { ZcodeAcpServer } from "../src/server.js"; + +vi.mock("../src/tasks-index.js", () => ({ + upsertSessionTask: async () => true, + updateSessionTitle: async () => true, +})); + +/** Fake backend whose session/resume returns the given workspace. */ +function fakeBackend( + history: ZcodeMessage[] = [], + resumeWorkspace?: string, +): { + backend: ZcodeBackend; + calls: Array<{ method: string; params: Record }>; +} { + const calls: Array<{ method: string; params: Record }> = []; + const backend = { + isDead: false, + request: async (_id: number, method: string, params: Record) => { + calls.push({ method, params }); + switch (method) { + case "workspace/updateProviderRegistry": + return { result: {} }; + case "session/resume": + return { + result: + resumeWorkspace === undefined + ? {} + : { session: { workspace: { workspacePath: resumeWorkspace } } }, + }; + case "session/subscribe": + return { result: { eventSeq: 0 } }; + case "session/read": + return { result: { projection: { status: "idle", contextUsed: 0 } } }; + case "session/messages": + return { result: { messages: history } }; + case "session/list": + return { result: { sessions: [] } }; + default: + return { result: {} }; + } + }, + send: () => {}, + pollServerRequests: () => [], + registerEventListener: () => {}, + unregisterEventListener: () => {}, + } as unknown as ZcodeBackend; + return { backend, calls }; +} + +const stubCx = { notify: async () => {} } as unknown as acp.AgentContext; + +function loadParams(cwd?: string): acp.LoadSessionRequest { + return { sessionId: "s-x", ...(cwd !== undefined ? { cwd } : {}) } as acp.LoadSessionRequest; +} + +describe("session/load cwd trust", () => { + it("ignores a client-supplied / — never records it as the root", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend(); + server.backend = backend; + server.registerSession("s-x", "sess_x"); + + await loadSession(server, loadParams("/"), stubCx); + + expect(server.sessionCwds.get("s-x")).not.toBe("/"); + }); + + it("ignores any client cwd for existing sessions — even plausible paths", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend(); + server.backend = backend; + server.registerSession("s-x", "sess_x"); + + await loadSession(server, loadParams("/Users/attacker/elsewhere"), stubCx); + + // Client value must NOT become the root; nothing recorded → process cwd. + expect(server.sessionCwds.get("s-x")).toBe(process.cwd()); + }); + + it("adopts the backend's own workspace from the resume result", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend([], "/Users/proj/backend-truth"); + server.backend = backend; + server.registerSession("s-x", "sess_x"); + + await loadSession(server, loadParams("/Users/attacker/elsewhere"), stubCx); + + expect(server.sessionCwds.get("s-x")).toBe("/Users/proj/backend-truth"); + }); + + it("heals a previously-polluted / entry from the backend workspace", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend([], "/Users/proj/backend-truth"); + server.backend = backend; + server.registerSession("s-x", "sess_x"); + server.sessionCwds.set("s-x", "/"); + + await loadSession(server, loadParams(), stubCx); + + expect(server.sessionCwds.get("s-x")).toBe("/Users/proj/backend-truth"); + }); + + it("keeps the recorded root when the session is already live (no resume RPC)", async () => { + const server = new ZcodeAcpServer(); + const { backend, calls } = fakeBackend(); + server.backend = backend; + server.registerSession("s-x", "sess_x"); + server.markBackendLoaded("s-x"); + server.sessionCwds.set("s-x", "/Users/proj/real"); + + await loadSession(server, loadParams("/"), stubCx); + + expect(server.sessionCwds.get("s-x")).toBe("/Users/proj/real"); + expect(calls.some((c) => c.method === "session/resume")).toBe(false); + }); +}); + +describe("newSession cwd guard", () => { + it("accepts the client worktree at creation", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend(); + server.backend = backend; + + const resp = await newSession(server, { + cwd: "/Users/proj/app", + mcpServers: [], + } as unknown as acp.NewSessionRequest); + + expect(server.sessionCwds.get(resp.sessionId)).toBe("/Users/proj/app"); + }); + + it("falls back to the process cwd instead of recording /", async () => { + const server = new ZcodeAcpServer(); + const { backend } = fakeBackend(); + server.backend = backend; + + const resp = await newSession(server, { + cwd: "/", + mcpServers: [], + } as unknown as acp.NewSessionRequest); + + expect(server.sessionCwds.get(resp.sessionId)).toBe(process.cwd()); + }); +}); + +describe("projectCwd()", () => { + it("prefers the most recently active session's cwd and skips / entries", () => { + const server = new ZcodeAcpServer(); + server.sessionCwds.set("s-stale", "/Users/proj/old"); + server.sessionCwds.set("s-polluted", "/"); + server.sessionCwds.set("s-fresh", "/Users/proj/new"); + server.sessionSummaries.set("s-stale", { updatedAt: 1_000 }); + server.sessionSummaries.set("s-polluted", { updatedAt: 2_000 }); + server.sessionSummaries.set("s-fresh", { updatedAt: 3_000 }); + + expect(server.projectCwd()).toBe("/Users/proj/new"); + }); + + it("falls back to the process cwd when every entry is polluted", () => { + const server = new ZcodeAcpServer(); + server.sessionCwds.set("s-a", "/"); + server.sessionCwds.set("s-b", "/"); + + expect(server.projectCwd()).toBe(process.cwd()); + }); +}); diff --git a/tests/session-lazy.test.ts b/tests/session-lazy.test.ts index 25ac940..a2f5c5c 100644 --- a/tests/session-lazy.test.ts +++ b/tests/session-lazy.test.ts @@ -278,8 +278,10 @@ describe("resumeSession with lazy placeholders", () => { const resumes = calls.filter((c) => c.method === "session/resume"); expect(resumes).toHaveLength(1); expect(resumes[0].params).toMatchObject({ sessionId: "sess_real_1", mcpServers }); - // The resume cwd becomes the session root for remote file access. - expect(server.sessionCwds.get("sess_real_1")).toBe("/tmp/ws"); + // The client cwd does NOT become the session root (backend-authoritative + // only); with no recorded root and no workspace in the resume result the + // bridge falls back to its process cwd. + expect(server.sessionCwds.get("sess_real_1")).toBe(process.cwd()); }); it("resumes an already-materialized placeholder without backend resume", async () => { @@ -383,8 +385,8 @@ describe("loadSession with lazy placeholders", () => { const resume = calls.find((c) => c.method === "session/resume"); expect(resume?.params).toMatchObject({ sessionId: "sess_real" }); - // Same as resumeSession: the load cwd is recorded as the session root. - expect(server.sessionCwds.get("sess_real")).toBe("/tmp/ws"); + // Same as resumeSession: the client cwd is NOT recorded as the root. + expect(server.sessionCwds.get("sess_real")).toBe(process.cwd()); }); });