mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 19:50:43 +00:00
fix(coven): tighten security hardening
This commit is contained in:
@@ -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/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<HttpResponse> {
|
||||
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<T>(options: RequestOptions): Promise<T> {
|
||||
}
|
||||
}
|
||||
|
||||
export function createCovenClient(socketPath: string): CovenClient {
|
||||
export function createCovenClient(
|
||||
socketPath: string,
|
||||
clientOptions: { socketRoot?: string } = {},
|
||||
): CovenClient {
|
||||
return {
|
||||
health(signal) {
|
||||
return requestJson<CovenHealthResponse>({
|
||||
socketPath,
|
||||
socketRoot: clientOptions.socketRoot,
|
||||
method: "GET",
|
||||
path: "/health",
|
||||
signal,
|
||||
@@ -181,6 +222,7 @@ export function createCovenClient(socketPath: string): CovenClient {
|
||||
launchSession(input, signal) {
|
||||
return requestJson<CovenSessionRecord>({
|
||||
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<CovenSessionRecord>({
|
||||
socketPath,
|
||||
socketRoot: clientOptions.socketRoot,
|
||||
method: "GET",
|
||||
path: `/sessions/${encodeURIComponent(sessionId)}`,
|
||||
signal,
|
||||
@@ -203,6 +246,7 @@ export function createCovenClient(socketPath: string): CovenClient {
|
||||
}
|
||||
return requestJson<CovenEventRecord[]>({
|
||||
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<unknown>({
|
||||
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<unknown>({
|
||||
socketPath,
|
||||
socketRoot: clientOptions.socketRoot,
|
||||
method: "POST",
|
||||
path: `/sessions/${encodeURIComponent(sessionId)}/kill`,
|
||||
signal,
|
||||
|
||||
@@ -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";
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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 });
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import {
|
||||
AcpRuntimeError,
|
||||
@@ -32,6 +33,7 @@ const DEFAULT_HARNESSES: Record<string, string> = {
|
||||
};
|
||||
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<CovenRuntimeSessionState>;
|
||||
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<CovenRuntimeSessionState>;
|
||||
const agent = normalizeAgentId(typeof parsed.agent === "string" ? parsed.agent : undefined);
|
||||
return {
|
||||
agent,
|
||||
@@ -95,7 +103,7 @@ function defaultSleep(ms: number, signal?: AbortSignal): Promise<void> {
|
||||
}
|
||||
|
||||
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<void> {
|
||||
@@ -483,5 +527,7 @@ export const __testing = {
|
||||
decodeRuntimeSessionName,
|
||||
encodeRuntimeSessionName,
|
||||
eventToRuntimeEvents,
|
||||
normalizeStopReason,
|
||||
sanitizeTerminalText,
|
||||
titleFromPrompt,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user