From 97d38af353d32c87b44eb1d8a11075673e6c6cd1 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Mon, 27 Apr 2026 10:56:08 -0500 Subject: [PATCH] fix(coven): tighten security hardening --- extensions/coven/src/client.test.ts | 11 ++++ extensions/coven/src/client.ts | 48 +++++++++++++++++- extensions/coven/src/config.test.ts | 22 ++++++++ extensions/coven/src/config.ts | 30 +++++++++++ extensions/coven/src/runtime.test.ts | 67 +++++++++++++++++++++++- extensions/coven/src/runtime.ts | 76 ++++++++++++++++++++++------ 6 files changed, 236 insertions(+), 18 deletions(-) diff --git a/extensions/coven/src/client.test.ts b/extensions/coven/src/client.test.ts index c028d6e59ff..4f660659fa4 100644 --- a/extensions/coven/src/client.test.ts +++ b/extensions/coven/src/client.test.ts @@ -86,4 +86,15 @@ describe("createCovenClient", () => { }, ); }); + + it("revalidates socket paths before connecting", async () => { + const covenHome = path.join(tmpDir, ".coven"); + await fs.mkdir(covenHome); + const socketPath = path.join(covenHome, "coven.sock"); + await fs.symlink("/var/run/docker.sock", socketPath); + + await expect(createCovenClient(socketPath, { socketRoot: covenHome }).health()).rejects.toThrow( + /must not be a symlink/, + ); + }); }); diff --git a/extensions/coven/src/client.ts b/extensions/coven/src/client.ts index f96098fc4c6..03f159ad995 100644 --- a/extensions/coven/src/client.ts +++ b/extensions/coven/src/client.ts @@ -1,4 +1,6 @@ +import fs from "node:fs"; import http from "node:http"; +import path from "node:path"; export type CovenSessionRecord = { id: string; @@ -55,6 +57,7 @@ export type CovenListEventsOptions = { type RequestOptions = { socketPath: string; + socketRoot?: string; method: "GET" | "POST"; path: string; body?: unknown; @@ -81,12 +84,46 @@ export class CovenApiError extends Error { const DEFAULT_REQUEST_TIMEOUT_MS = 10_000; const MAX_RESPONSE_BYTES = 1_000_000; +function pathIsInside(parent: string, child: string): boolean { + const relative = path.relative(parent, child); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); +} + +function lstatIfExists(filePath: string): fs.Stats | null { + try { + return fs.lstatSync(filePath); + } catch { + return null; + } +} + +function validateSocketPathForUse(socketPath: string, socketRoot: string | undefined): void { + if (!socketRoot) { + return; + } + const socketStat = lstatIfExists(socketPath); + if (socketStat?.isSymbolicLink()) { + throw new Error("Coven socketPath must not be a symlink"); + } + const realSocketRoot = fs.realpathSync.native(socketRoot); + const realSocketDir = fs.realpathSync.native(path.dirname(socketPath)); + if (!pathIsInside(realSocketRoot, realSocketDir)) { + throw new Error("Coven socketPath must stay inside covenHome"); + } +} + function requestOverSocket(options: RequestOptions): Promise { return new Promise((resolve, reject) => { if (options.signal?.aborted) { reject(options.signal.reason ?? new Error("request aborted")); return; } + try { + validateSocketPathForUse(options.socketPath, options.socketRoot); + } catch (error) { + reject(error); + return; + } let settled = false; let body = ""; @@ -168,11 +205,15 @@ async function requestJson(options: RequestOptions): Promise { } } -export function createCovenClient(socketPath: string): CovenClient { +export function createCovenClient( + socketPath: string, + clientOptions: { socketRoot?: string } = {}, +): CovenClient { return { health(signal) { return requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "GET", path: "/health", signal, @@ -181,6 +222,7 @@ export function createCovenClient(socketPath: string): CovenClient { launchSession(input, signal) { return requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "POST", path: "/sessions", body: input, @@ -190,6 +232,7 @@ export function createCovenClient(socketPath: string): CovenClient { getSession(sessionId, signal) { return requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "GET", path: `/sessions/${encodeURIComponent(sessionId)}`, signal, @@ -203,6 +246,7 @@ export function createCovenClient(socketPath: string): CovenClient { } return requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "GET", path: `/events?${params.toString()}`, signal, @@ -211,6 +255,7 @@ export function createCovenClient(socketPath: string): CovenClient { async sendInput(sessionId, data, signal) { await requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "POST", path: `/sessions/${encodeURIComponent(sessionId)}/input`, body: { data }, @@ -220,6 +265,7 @@ export function createCovenClient(socketPath: string): CovenClient { async killSession(sessionId, signal) { await requestJson({ socketPath, + socketRoot: clientOptions.socketRoot, method: "POST", path: `/sessions/${encodeURIComponent(sessionId)}/kill`, signal, diff --git a/extensions/coven/src/config.test.ts b/extensions/coven/src/config.test.ts index 34bb4cd362c..88ff25a7ee0 100644 --- a/extensions/coven/src/config.test.ts +++ b/extensions/coven/src/config.test.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -53,6 +54,27 @@ describe("resolveCovenPluginConfig", () => { ).toThrow(/socketPath must stay inside covenHome/); }); + it("rejects socket paths that are symlinks", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-config-")); + const covenHome = path.join(workspaceDir, ".coven"); + await fs.mkdir(covenHome); + const socketPath = path.join(covenHome, "coven.sock"); + await fs.symlink("/var/run/docker.sock", socketPath); + try { + expect(() => + resolveCovenPluginConfig({ + rawConfig: { + covenHome, + socketPath, + }, + workspaceDir, + }), + ).toThrow(/must not be a symlink/); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }); + } + }); + it("uses COVEN_HOME with tilde expansion for the default socket path", () => { process.env.COVEN_HOME = "~/.custom-coven"; diff --git a/extensions/coven/src/config.ts b/extensions/coven/src/config.ts index bafc42142d6..3ff813dede8 100644 --- a/extensions/coven/src/config.ts +++ b/extensions/coven/src/config.ts @@ -1,3 +1,4 @@ +import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { buildPluginConfigSchema } from "openclaw/plugin-sdk/core"; @@ -63,6 +64,22 @@ function pathIsInside(parent: string, child: string): boolean { return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); } +function realpathIfExists(filePath: string): string | null { + try { + return fs.realpathSync.native(filePath); + } catch { + return null; + } +} + +function lstatIfExists(filePath: string): fs.Stats | null { + try { + return fs.lstatSync(filePath); + } catch { + return null; + } +} + function resolveCovenHome(raw: string | undefined, baseDir: string): string { const fromConfig = raw?.trim(); if (fromConfig) { @@ -82,6 +99,19 @@ function resolveSocketPath(covenHome: string, raw: string | undefined, baseDir: if (!pathIsInside(covenHome, socketPath)) { throw new Error("Coven socketPath must stay inside covenHome"); } + const socketStat = lstatIfExists(socketPath); + if (socketStat?.isSymbolicLink()) { + throw new Error("Coven socketPath must not be a symlink"); + } + const realCovenHome = realpathIfExists(covenHome); + const realSocketDir = realpathIfExists(path.dirname(socketPath)); + if (realCovenHome && realSocketDir && !pathIsInside(realCovenHome, realSocketDir)) { + throw new Error("Coven socketPath must stay inside covenHome"); + } + const realSocketPath = realpathIfExists(socketPath); + if (realCovenHome && realSocketPath && !pathIsInside(realCovenHome, realSocketPath)) { + throw new Error("Coven socketPath must stay inside covenHome"); + } return socketPath; } diff --git a/extensions/coven/src/runtime.test.ts b/extensions/coven/src/runtime.test.ts index 0104c74d786..1ea13b3f734 100644 --- a/extensions/coven/src/runtime.test.ts +++ b/extensions/coven/src/runtime.test.ts @@ -1,3 +1,6 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { registerAcpRuntimeBackend, unregisterAcpRuntimeBackend, @@ -239,6 +242,39 @@ describe("CovenAcpRuntime", () => { ).rejects.toThrow(/outside workspace/); }); + it("rejects Coven cwd symlinks that resolve outside the workspace", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-workspace-")); + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-outside-")); + const symlinkPath = path.join(workspaceDir, "outside"); + await fs.symlink(outsideDir, symlinkPath); + try { + const runtime = new CovenAcpRuntime({ + config: { ...config, workspaceDir }, + client: fakeClient(), + }); + const handle = await runtime.ensureSession({ + sessionKey: "agent:codex:test", + agent: "codex", + mode: "oneshot", + cwd: symlinkPath, + }); + + await expect( + collect( + runtime.runTurn({ + handle, + text: "Fix tests", + mode: "prompt", + requestId: "req-1", + }), + ), + ).rejects.toThrow(/outside workspace/); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + it("requests incremental events after the last processed Coven event", async () => { const client = fakeClient({ listEvents: vi @@ -309,11 +345,38 @@ describe("CovenAcpRuntime", () => { }); it("strips terminal escape and control characters from Coven output", () => { - expect(__testing.sanitizeTerminalText("\u001b]0;spoof\u0007hi\u001b[31m!\u001b[0m\r\n")).toBe( - "hi!\n", + expect( + __testing.sanitizeTerminalText( + "\u001b]0;spoof\u0007hi\u001b[31m!\u001b[0m\u001b7\u001bc\r\n", + ), + ).toBe("hi!\n"); + }); + + it("sanitizes prompt-derived session titles", () => { + expect(__testing.titleFromPrompt("\u001b]0;spoof\u0007Fix\u001b[31m tests\r\nnow")).toBe( + "Fix tests now", ); }); + it("normalizes untrusted Coven exit status into bounded stop reasons", () => { + expect(__testing.normalizeStopReason("completed")).toBe("completed"); + expect(__testing.normalizeStopReason("killed")).toBe("cancelled"); + expect(__testing.normalizeStopReason("refusal")).toBe("completed"); + + expect( + __testing.eventToRuntimeEvents( + event({ + kind: "exit", + payloadJson: JSON.stringify({ status: "refusal", exitCode: 0 }), + }), + ), + ).toContainEqual(expect.objectContaining({ type: "done", stopReason: "completed" })); + }); + + it("rejects oversized Coven runtime session metadata", () => { + expect(__testing.decodeRuntimeSessionName(`coven:${"a".repeat(2_049)}`)).toBeNull(); + }); + it("preserves direct fallback when Coven launch fails after detection", async () => { const fallback = fallbackRuntime(); registerAcpRuntimeBackend({ id: "acpx", runtime: fallback }); diff --git a/extensions/coven/src/runtime.ts b/extensions/coven/src/runtime.ts index 7fa5dfcc0e8..76f50597236 100644 --- a/extensions/coven/src/runtime.ts +++ b/extensions/coven/src/runtime.ts @@ -1,3 +1,4 @@ +import fs from "node:fs"; import path from "node:path"; import { AcpRuntimeError, @@ -32,6 +33,7 @@ const DEFAULT_HARNESSES: Record = { }; const HEALTH_CHECK_TIMEOUT_MS = 5_000; const MAX_TRACKED_EVENT_IDS = 10_000; +const MAX_RUNTIME_SESSION_NAME_BYTES = 2_048; type CovenRuntimeSessionState = { agent: string; @@ -57,13 +59,19 @@ function encodeRuntimeSessionName(state: CovenRuntimeSessionState): string { function decodeRuntimeSessionName(value: string): CovenRuntimeSessionState | null { const encoded = value.startsWith("coven:") ? value.slice("coven:".length) : ""; - if (!encoded) { + if (!encoded || encoded.length > MAX_RUNTIME_SESSION_NAME_BYTES) { return null; } try { - const parsed = JSON.parse( - Buffer.from(encoded, "base64url").toString("utf8"), - ) as Partial; + const decoded = Buffer.from(encoded, "base64url"); + if (decoded.byteLength > MAX_RUNTIME_SESSION_NAME_BYTES) { + return null; + } + const jsonText = decoded.toString("utf8"); + if (Buffer.byteLength(jsonText, "utf8") > MAX_RUNTIME_SESSION_NAME_BYTES) { + return null; + } + const parsed = JSON.parse(jsonText) as Partial; const agent = normalizeAgentId(typeof parsed.agent === "string" ? parsed.agent : undefined); return { agent, @@ -95,7 +103,7 @@ function defaultSleep(ms: number, signal?: AbortSignal): Promise { } function titleFromPrompt(prompt: string): string { - const compact = prompt.replace(/\s+/g, " ").trim(); + const compact = sanitizeStatusText(prompt); return compact.slice(0, 80) || "OpenClaw task"; } @@ -118,7 +126,7 @@ const del = String.fromCharCode(0x7f); const c1Start = String.fromCharCode(0x80); const c1End = String.fromCharCode(0x9f); const ANSI_ESCAPE_REGEX = new RegExp( - `${ESC}(?:\\[[\\x20-\\x3f]*[\\x40-\\x7e]|\\][^${BEL}${ESC}]*(?:${BEL}|${ESC}\\\\)|[\\x40-\\x5f])`, + `${ESC}(?:\\][\\s\\S]*?(?:${BEL}|${ESC}\\\\)|P[\\s\\S]*?${ESC}\\\\|\\[[\\x20-\\x3f]*[\\x40-\\x7e]|[\\x20-\\x2f]*[\\x30-\\x7e])`, "g", ); const TEXT_CONTROL_REGEX = new RegExp( @@ -130,6 +138,25 @@ function sanitizeTerminalText(input: string): string { return input.replace(ANSI_ESCAPE_REGEX, "").replace(TEXT_CONTROL_REGEX, ""); } +function sanitizeStatusText(input: string): string { + return sanitizeTerminalText(input).replace(/\s+/g, " ").trim(); +} + +function normalizeStopReason(value: unknown): string { + const normalized = + typeof value === "string" ? sanitizeStatusText(value).toLowerCase() : "completed"; + if (normalized === "completed" || normalized === "complete" || normalized === "success") { + return "completed"; + } + if (normalized === "killed" || normalized === "cancelled" || normalized === "canceled") { + return "cancelled"; + } + if (normalized === "failed" || normalized === "failure" || normalized === "error") { + return "error"; + } + return "completed"; +} + function eventToRuntimeEvents(event: CovenEventRecord): AcpRuntimeEvent[] { const payload = parsePayload(event); if (event.kind === "output") { @@ -137,7 +164,9 @@ function eventToRuntimeEvents(event: CovenEventRecord): AcpRuntimeEvent[] { return text ? [{ type: "text_delta", text, stream: "output", tag: "agent_message_chunk" }] : []; } if (event.kind === "exit") { - const status = typeof payload.status === "string" ? payload.status : "completed"; + const status = sanitizeStatusText( + typeof payload.status === "string" ? payload.status : "completed", + ); const exitCode = typeof payload.exitCode === "number" ? payload.exitCode : null; return [ { @@ -145,13 +174,13 @@ function eventToRuntimeEvents(event: CovenEventRecord): AcpRuntimeEvent[] { text: `coven session ${status}${exitCode == null ? "" : ` exitCode=${exitCode}`}`, tag: "session_info_update", }, - { type: "done", stopReason: status }, + { type: "done", stopReason: normalizeStopReason(status) }, ]; } if (event.kind === "kill") { return [ { type: "status", text: "coven session killed", tag: "session_info_update" }, - { type: "done", stopReason: "killed" }, + { type: "done", stopReason: "cancelled" }, ]; } return []; @@ -162,9 +191,10 @@ function sessionIsTerminal(session: CovenSessionRecord): boolean { } function terminalStatusEvent(session: CovenSessionRecord): AcpRuntimeEvent { + const status = sanitizeStatusText(session.status); return { type: "status", - text: `coven session ${session.status}${session.exitCode == null ? "" : ` exitCode=${session.exitCode}`}`, + text: `coven session ${status}${session.exitCode == null ? "" : ` exitCode=${session.exitCode}`}`, tag: "session_info_update", }; } @@ -174,6 +204,14 @@ function pathIsInside(parent: string, child: string): boolean { return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); } +function realpathIfExists(filePath: string): string | null { + try { + return fs.realpathSync.native(filePath); + } catch { + return null; + } +} + export class CovenAcpRuntime implements AcpRuntime { private readonly config: ResolvedCovenPluginConfig; private readonly client: CovenClient; @@ -184,7 +222,9 @@ export class CovenAcpRuntime implements AcpRuntime { constructor(params: CovenAcpRuntimeParams) { this.config = params.config; this.logger = params.logger; - this.client = params.client ?? createCovenClient(params.config.socketPath); + this.client = + params.client ?? + createCovenClient(params.config.socketPath, { socketRoot: params.config.covenHome }); this.sleep = params.sleep ?? defaultSleep; } @@ -295,7 +335,7 @@ export class CovenAcpRuntime implements AcpRuntime { const latest = await this.client.getSession(session.id, input.signal); if (sessionIsTerminal(latest)) { yield terminalStatusEvent(latest); - yield { type: "done", stopReason: latest.status }; + yield { type: "done", stopReason: normalizeStopReason(latest.status) }; this.activeSessionIdsBySessionKey.delete(input.handle.sessionKey); return; } @@ -337,7 +377,7 @@ export class CovenAcpRuntime implements AcpRuntime { } const session = await this.client.getSession(sessionId, input.signal); return { - summary: `${session.status} ${session.harness} ${session.title}`, + summary: `${sanitizeStatusText(session.status)} ${sanitizeStatusText(session.harness)} ${sanitizeStatusText(session.title)}`, backendSessionId: session.id, agentSessionId: session.id, details: { @@ -468,10 +508,14 @@ export class CovenAcpRuntime implements AcpRuntime { private resolveWorkspaceCwd(candidate: string | undefined): string { const cwd = path.resolve(candidate ?? this.config.workspaceDir); - if (!pathIsInside(this.config.workspaceDir, cwd)) { + const workspaceReal = realpathIfExists(this.config.workspaceDir); + const cwdReal = realpathIfExists(cwd); + const boundary = workspaceReal ?? this.config.workspaceDir; + const checkedCwd = cwdReal ?? cwd; + if (!pathIsInside(boundary, checkedCwd)) { throw new AcpRuntimeError("ACP_SESSION_INIT_FAILED", "Coven cwd is outside workspace."); } - return cwd; + return checkedCwd; } private async killActiveSession(sessionId: string, signal?: AbortSignal): Promise { @@ -483,5 +527,7 @@ export const __testing = { decodeRuntimeSessionName, encodeRuntimeSessionName, eventToRuntimeEvents, + normalizeStopReason, sanitizeTerminalText, + titleFromPrompt, };