diff --git a/docs/tools/acp-agents-setup.md b/docs/tools/acp-agents-setup.md index 7f71fdb8bb8..863fa31eedd 100644 --- a/docs/tools/acp-agents-setup.md +++ b/docs/tools/acp-agents-setup.md @@ -216,7 +216,7 @@ Minimal opt-in config: coven: { enabled: true, config: { - // Optional. Defaults to COVEN_HOME or ~/.coven. + // Optional. Defaults to ~/.coven. Environment variables are not used for this trust anchor. covenHome: "~/.coven", // Optional. Defaults to /coven.sock; overrides must resolve to that path. socketPath: "~/.coven/coven.sock", @@ -246,7 +246,9 @@ daemon socket is a local user trust anchor, not repository-controlled state. arbitrary Coven socket filenames because the daemon socket is the local trust anchor. Keep `covenHome` owned by the OpenClaw user and private (`0700`); OpenClaw rejects symlinked, shared-accessible, shared-writable, or non-socket -Coven socket paths before connecting. +Coven socket paths before connecting. The Coven backend currently requires Unix +socket validation and fails closed on Windows rather than trusting a socket path +whose owner and permissions cannot be validated by this plugin. The default harness mapping sends known ACP agent ids such as `codex`, `claude`, `gemini`, and `opencode` to explicitly authorized Coven harness ids. Unknown diff --git a/extensions/coven/openclaw.plugin.json b/extensions/coven/openclaw.plugin.json index 0bbcbd404bc..fd347e3a32a 100644 --- a/extensions/coven/openclaw.plugin.json +++ b/extensions/coven/openclaw.plugin.json @@ -9,7 +9,7 @@ "properties": { "covenHome": { "type": "string", - "description": "Path to COVEN_HOME. Defaults to COVEN_HOME or ~/.coven." + "description": "Path to the Coven daemon state directory. Defaults to ~/.coven; environment variables are not used for this trust anchor." }, "socketPath": { "type": "string", diff --git a/extensions/coven/src/client.test.ts b/extensions/coven/src/client.test.ts index 75aac1e4994..4682a92e272 100644 --- a/extensions/coven/src/client.test.ts +++ b/extensions/coven/src/client.test.ts @@ -3,7 +3,7 @@ import http from "node:http"; import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { CovenApiError, createCovenClient } from "./client.js"; +import { __testing, CovenApiError, createCovenClient } from "./client.js"; let tmpDir: string; @@ -211,4 +211,14 @@ describe("createCovenClient", () => { }); } }); + + it("fails closed instead of bypassing socket validation on Windows", () => { + expect(() => + __testing.validateSocketPathForUse( + path.join(tmpDir, ".coven", "coven.sock"), + path.join(tmpDir, ".coven"), + "win32", + ), + ).toThrow(/not supported on Windows/); + }); }); diff --git a/extensions/coven/src/client.ts b/extensions/coven/src/client.ts index 90b5104473b..3887893800d 100644 --- a/extensions/coven/src/client.ts +++ b/extensions/coven/src/client.ts @@ -137,17 +137,19 @@ function socketFingerprintMatches(left: SocketFingerprint, right: SocketFingerpr function validateSocketPathForUse( socketPath: string, socketRoot: string | undefined, + platform: NodeJS.Platform = process.platform, ): SocketFingerprint | null { if (!socketRoot) { return null; } + validateSocketPlatform(platform); const socketRootLstat = lstatIfExists(socketRoot); if (socketRootLstat?.isSymbolicLink()) { throw new Error("Coven covenHome must not be a symlink"); } const socketRootStat = statExistingPath(socketRoot, "Coven covenHome"); - validateSocketOwnerAndMode(socketRootStat, "Coven covenHome"); - validatePrivateDirectory(socketRootStat, "Coven covenHome"); + validateSocketOwnerAndMode(socketRootStat, "Coven covenHome", platform); + validatePrivateDirectory(socketRootStat, "Coven covenHome", platform); const expectedSocketPath = path.resolve(socketRoot, DEFAULT_SOCKET_FILENAME); if (path.resolve(socketPath) !== expectedSocketPath) { throw new Error("Coven socketPath must be /coven.sock"); @@ -161,7 +163,7 @@ function validateSocketPathForUse( if (!resolvedSocketStat.isSocket()) { throw new Error("Coven socketPath must be a Unix socket"); } - validateSocketOwnerAndMode(resolvedSocketStat, "Coven socketPath"); + validateSocketOwnerAndMode(resolvedSocketStat, "Coven socketPath", platform); const realSocketRoot = realpathExistingPath(socketRoot, "Coven covenHome"); const realSocketDir = realpathExistingPath( @@ -169,8 +171,8 @@ function validateSocketPathForUse( "Coven socketPath directory", ); const socketDirStat = statExistingPath(path.dirname(socketPath), "Coven socketPath directory"); - validateSocketOwnerAndMode(socketDirStat, "Coven socketPath directory"); - validatePrivateDirectory(socketDirStat, "Coven socketPath directory"); + validateSocketOwnerAndMode(socketDirStat, "Coven socketPath directory", platform); + validatePrivateDirectory(socketDirStat, "Coven socketPath directory", platform); if (!pathIsInside(realSocketRoot, realSocketDir)) { throw new Error("Coven socketPath must stay inside covenHome"); } @@ -181,6 +183,12 @@ function validateSocketPathForUse( return fingerprintSocket(resolvedSocketStat); } +function validateSocketPlatform(platform: NodeJS.Platform): void { + if (platform === "win32") { + throw new Error("Coven Unix socket validation is not supported on Windows"); + } +} + function requireSafeQueryId(input: string, label: string): string { const value = input.trim(); if (!value || value.length > MAX_QUERY_ID_CHARS || !SAFE_QUERY_ID_REGEX.test(value)) { @@ -189,10 +197,12 @@ function requireSafeQueryId(input: string, label: string): string { return value; } -function validateSocketOwnerAndMode(stat: fs.Stats, label: string): void { - if (process.platform === "win32") { - return; - } +function validateSocketOwnerAndMode( + stat: fs.Stats, + label: string, + platform: NodeJS.Platform, +): void { + validateSocketPlatform(platform); const currentUid = typeof process.getuid === "function" ? process.getuid() : null; if (currentUid != null && stat.uid !== currentUid) { throw new Error(`${label} must be owned by the current user`); @@ -202,10 +212,8 @@ function validateSocketOwnerAndMode(stat: fs.Stats, label: string): void { } } -function validatePrivateDirectory(stat: fs.Stats, label: string): void { - if (process.platform === "win32") { - return; - } +function validatePrivateDirectory(stat: fs.Stats, label: string, platform: NodeJS.Platform): void { + validateSocketPlatform(platform); if (!stat.isDirectory()) { throw new Error(`${label} must be a directory`); } @@ -426,3 +434,7 @@ export function createCovenClient( }, }; } + +export const __testing = { + validateSocketPathForUse, +}; diff --git a/extensions/coven/src/config.test.ts b/extensions/coven/src/config.test.ts index 2e2a61b60ca..44a5cb661a6 100644 --- a/extensions/coven/src/config.test.ts +++ b/extensions/coven/src/config.test.ts @@ -105,7 +105,7 @@ describe("resolveCovenPluginConfig", () => { } }); - it("uses COVEN_HOME with tilde expansion for the default socket path", () => { + it("ignores COVEN_HOME when resolving the socket trust anchor", () => { process.env.COVEN_HOME = "~/.custom-coven"; const resolved = resolveCovenPluginConfig({ @@ -113,8 +113,8 @@ describe("resolveCovenPluginConfig", () => { workspaceDir: "/repo", }); - expect(resolved.covenHome).toBe(path.join(os.homedir(), ".custom-coven")); - expect(resolved.socketPath).toBe(path.join(os.homedir(), ".custom-coven", "coven.sock")); + expect(resolved.covenHome).toBe(path.join(os.homedir(), ".coven")); + expect(resolved.socketPath).toBe(path.join(os.homedir(), ".coven", "coven.sock")); expect(resolved.allowFallback).toBe(false); }); diff --git a/extensions/coven/src/config.ts b/extensions/coven/src/config.ts index befcd77c4b0..e6206b7f326 100644 --- a/extensions/coven/src/config.ts +++ b/extensions/coven/src/config.ts @@ -71,10 +71,6 @@ function resolveCovenHome(raw: string | undefined): string { if (fromConfig) { return resolveConfiguredPath(fromConfig, "covenHome"); } - const fromEnv = process.env.COVEN_HOME?.trim(); - if (fromEnv) { - return resolveConfiguredPath(fromEnv, "covenHome"); - } return path.join(os.homedir(), ".coven"); } diff --git a/extensions/coven/src/runtime.test.ts b/extensions/coven/src/runtime.test.ts index f499020babc..107684fe09c 100644 --- a/extensions/coven/src/runtime.test.ts +++ b/extensions/coven/src/runtime.test.ts @@ -327,6 +327,39 @@ describe("CovenAcpRuntime", () => { ).rejects.toThrow(/session id is invalid/); expect(handle.backendSessionId).toBeUndefined(); expect(handle.agentSessionId).toBeUndefined(); + expect(client.killSession).toHaveBeenCalledWith("\u001b]0;spoof\u0007session-1\r", undefined); + }); + + it("kills an already-launched Coven session before falling back on unsafe session ids", async () => { + const fallback = fallbackRuntime(); + registerAcpRuntimeBackend({ id: "acpx", runtime: fallback }); + const client = fakeClient({ + launchSession: vi.fn(async () => session({ id: "bad\nsession" })), + killSession: vi.fn(async () => undefined), + }); + const runtime = new CovenAcpRuntime({ config: { ...config, allowFallback: true }, client }); + const handle = await runtime.ensureSession({ + sessionKey: "agent:codex:test", + agent: "codex", + mode: "oneshot", + cwd: workspaceDir, + }); + + const events = await collect( + runtime.runTurn({ + handle, + text: "Fix tests", + mode: "prompt", + requestId: "req-1", + }), + ); + + expect(client.killSession).toHaveBeenCalledWith("bad\nsession", undefined); + expect(handle.backend).toBe("acpx"); + expect(events).toEqual([ + expect.objectContaining({ type: "text_delta", text: "direct fallback\n" }), + expect.objectContaining({ type: "done", stopReason: "complete" }), + ]); }); it("fails closed without launching Coven when prompts exceed the Coven request limit", async () => { @@ -639,6 +672,29 @@ describe("CovenAcpRuntime", () => { ]); }); + it("sanitizes Coven polling errors before logging", async () => { + const logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }; + const client = fakeClient({ + listEvents: vi.fn(async () => { + throw new Error("\u001b]0;spoof\u0007bad\r\njson"); + }), + killSession: vi.fn(async () => undefined), + }); + const runtime = new CovenAcpRuntime({ config, client, logger }); + const handle = await runtime.ensureSession({ + sessionKey: "agent:codex:test", + agent: "codex", + mode: "oneshot", + cwd: workspaceDir, + }); + + await collect( + runtime.runTurn({ handle, text: "Fix tests", mode: "prompt", requestId: "req-1" }), + ); + + expect(logger.warn).toHaveBeenCalledWith("coven polling failed: Error: bad json"); + }); + it("strips terminal escape and control characters from Coven output", () => { expect( __testing.sanitizeTerminalText( @@ -841,7 +897,7 @@ describe("CovenAcpRuntime", () => { config, client: fakeClient({ launchSession: vi.fn(async () => { - throw new Error("launch failed"); + throw new Error("\u001b]0;spoof\u0007launch\r\nfailed"); }), }), }); @@ -854,6 +910,22 @@ describe("CovenAcpRuntime", () => { await expect( collect(runtime.runTurn({ handle, text: "Fix tests", mode: "prompt", requestId: "req-1" })), - ).rejects.toThrow(/fallback is disabled/); + ).rejects.toThrow(/Error: launch failed/); + }); + + it("sanitizes Coven doctor error details", async () => { + const runtime = new CovenAcpRuntime({ + config, + client: fakeClient({ + health: vi.fn(async () => { + throw new Error("\u001b[31moffline\r\nnow"); + }), + }), + }); + + await expect(runtime.doctor()).resolves.toMatchObject({ + ok: false, + details: ["Error: offline now"], + }); }); }); diff --git a/extensions/coven/src/runtime.ts b/extensions/coven/src/runtime.ts index 0bb62011d14..03c131be47c 100644 --- a/extensions/coven/src/runtime.ts +++ b/extensions/coven/src/runtime.ts @@ -184,6 +184,11 @@ function sanitizeStatusField(input: string, fallback = "unknown"): string { return sanitizeStatusText(input).slice(0, MAX_STATUS_FIELD_CHARS) || fallback; } +function sanitizeErrorText(error: unknown): string { + const raw = error instanceof Error ? `${error.name}: ${error.message}` : String(error); + return sanitizeStatusField(raw, "unknown error"); +} + function requireSafeSessionId(input: string): string { const value = input.trim(); if (!value || value.length > MAX_SESSION_ID_CHARS || !SAFE_SESSION_ID_REGEX.test(value)) { @@ -333,7 +338,7 @@ export class CovenAcpRuntime implements AcpRuntime { const cwd = this.resolveWorkspaceCwd(input.handle.cwd); const harness = this.resolveHarness(state.agent); - let session: CovenSessionRecord; + let session: CovenSessionRecord | undefined; let sessionId: string; try { const prompt = boundedCovenPrompt(input.text); @@ -347,17 +352,38 @@ export class CovenAcpRuntime implements AcpRuntime { }, input.signal, ); - sessionId = requireSafeSessionId(session.id); } catch (error) { + const safeError = sanitizeErrorText(error); if (!this.config.allowFallback) { throw new AcpRuntimeError( "ACP_TURN_FAILED", - `Coven launch failed and fallback is disabled: ${String(error)}`, + `Coven launch failed and fallback is disabled: ${safeError}`, { cause: error }, ); } this.logger?.warn( - `coven launch failed; falling back to ${this.config.fallbackBackend}: ${String(error)}`, + `coven launch failed; falling back to ${this.config.fallbackBackend}: ${safeError}`, + ); + yield* this.runFallbackFromCovenHandle(input, state); + return; + } + try { + if (!session) { + throw new Error("Coven launch did not return a session"); + } + sessionId = requireSafeSessionId(session.id); + } catch (error) { + await this.killLaunchedSessionBestEffort(session?.id); + const safeError = sanitizeErrorText(error); + if (!this.config.allowFallback) { + throw new AcpRuntimeError( + "ACP_TURN_FAILED", + `Coven launch failed and fallback is disabled: ${safeError}`, + { cause: error }, + ); + } + this.logger?.warn( + `coven launch failed; falling back to ${this.config.fallbackBackend}: ${safeError}`, ); yield* this.runFallbackFromCovenHandle(input, state); return; @@ -425,7 +451,7 @@ export class CovenAcpRuntime implements AcpRuntime { await this.killActiveSession(sessionId).catch(() => undefined); throw input.signal.reason ?? error; } - this.logger?.warn(`coven polling failed: ${String(error)}`); + this.logger?.warn(`coven polling failed: ${sanitizeErrorText(error)}`); await this.killActiveSession(sessionId).catch(() => undefined); this.activeSessionIdsBySessionKey.delete(input.handle.sessionKey); yield { type: "status", text: "coven session polling failed", tag: "session_info_update" }; @@ -482,7 +508,7 @@ export class CovenAcpRuntime implements AcpRuntime { ok: false, code: "COVEN_UNAVAILABLE", message: "Coven daemon is not reachable; direct ACP fallback remains available.", - details: [String(error)], + details: [sanitizeErrorText(error)], }; } } @@ -628,6 +654,13 @@ export class CovenAcpRuntime implements AcpRuntime { private async killActiveSession(sessionId: string, signal?: AbortSignal): Promise { await this.client.killSession(sessionId, signal); } + + private async killLaunchedSessionBestEffort(sessionId: string | undefined): Promise { + if (!sessionId) { + return; + } + await this.client.killSession(sessionId, undefined).catch(() => undefined); + } } export const __testing = { @@ -635,6 +668,7 @@ export const __testing = { encodeRuntimeSessionName, eventToRuntimeEvents, normalizeStopReason, + sanitizeErrorText, sanitizeStatusField, sanitizeTerminalText, terminalStatusEvent,