mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 19:50:43 +00:00
fix(coven): harden daemon request boundaries
This commit is contained in:
@@ -239,7 +239,9 @@ For path safety, `~` in `covenHome` and `socketPath` expands to the current
|
||||
user home directory. Relative Coven paths resolve from the OpenClaw workspace,
|
||||
not from the process working directory. `socketPath` must stay inside
|
||||
`covenHome`; use the default `<covenHome>/coven.sock` unless your Coven daemon
|
||||
uses a different socket filename in the same home directory.
|
||||
uses a different socket filename in the same home directory. Keep `covenHome`
|
||||
owned by the OpenClaw user and not group/world-writable; OpenClaw rejects
|
||||
symlinked, shared-writable, or non-socket Coven socket paths before connecting.
|
||||
|
||||
The default harness mapping sends common ACP agent ids such as `codex`,
|
||||
`claude`, `gemini`, and `opencode` to the matching Coven harness id. Override
|
||||
|
||||
@@ -50,6 +50,23 @@ describe("createCovenClient", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("validates a real socket inside the configured socket root", async () => {
|
||||
await withServer(
|
||||
(_req, res) => {
|
||||
res.setHeader("Content-Type", "application/json");
|
||||
res.end(JSON.stringify({ ok: true, daemon: null }));
|
||||
},
|
||||
async (socketPath) => {
|
||||
await expect(
|
||||
createCovenClient(socketPath, { socketRoot: tmpDir }).health(),
|
||||
).resolves.toEqual({
|
||||
ok: true,
|
||||
daemon: null,
|
||||
});
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
it("sends the event cursor when listing events", async () => {
|
||||
await withServer(
|
||||
(req, res) => {
|
||||
@@ -87,6 +104,25 @@ describe("createCovenClient", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects request bodies above the request size limit", async () => {
|
||||
await withServer(
|
||||
(_req, res) => {
|
||||
res.end("{}");
|
||||
},
|
||||
async (socketPath) => {
|
||||
await expect(
|
||||
createCovenClient(socketPath).launchSession({
|
||||
projectRoot: "/repo",
|
||||
cwd: "/repo",
|
||||
harness: "codex",
|
||||
prompt: "x".repeat(1_000_001),
|
||||
title: "Large prompt",
|
||||
}),
|
||||
).rejects.toThrow(/request exceeded size limit/);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
it("revalidates socket paths before connecting", async () => {
|
||||
const covenHome = path.join(tmpDir, ".coven");
|
||||
await fs.mkdir(covenHome);
|
||||
@@ -108,4 +144,28 @@ describe("createCovenClient", () => {
|
||||
createCovenClient(path.join(symlinkHome, "coven.sock"), { socketRoot: symlinkHome }).health(),
|
||||
).rejects.toThrow(/covenHome must not be a symlink/);
|
||||
});
|
||||
|
||||
it("rejects a group or world writable socket root", async () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const covenHome = path.join(tmpDir, ".coven");
|
||||
await fs.mkdir(covenHome);
|
||||
await fs.chmod(covenHome, 0o777);
|
||||
|
||||
await expect(
|
||||
createCovenClient(path.join(covenHome, "coven.sock"), { socketRoot: covenHome }).health(),
|
||||
).rejects.toThrow(/covenHome must not be group or world writable/);
|
||||
});
|
||||
|
||||
it("rejects socket paths that are not Unix sockets", async () => {
|
||||
const covenHome = path.join(tmpDir, ".coven");
|
||||
await fs.mkdir(covenHome);
|
||||
const socketPath = path.join(covenHome, "coven.sock");
|
||||
await fs.writeFile(socketPath, "");
|
||||
|
||||
await expect(createCovenClient(socketPath, { socketRoot: covenHome }).health()).rejects.toThrow(
|
||||
/must be a Unix socket/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import fs from "node:fs";
|
||||
import http from "node:http";
|
||||
import net from "node:net";
|
||||
import path from "node:path";
|
||||
|
||||
export type CovenSessionRecord = {
|
||||
@@ -82,6 +83,7 @@ export class CovenApiError extends Error {
|
||||
}
|
||||
|
||||
const DEFAULT_REQUEST_TIMEOUT_MS = 10_000;
|
||||
const MAX_REQUEST_BYTES = 1_000_000;
|
||||
const MAX_RESPONSE_BYTES = 1_000_000;
|
||||
|
||||
function pathIsInside(parent: string, child: string): boolean {
|
||||
@@ -101,13 +103,23 @@ function validateSocketPathForUse(socketPath: string, socketRoot: string | undef
|
||||
if (!socketRoot) {
|
||||
return;
|
||||
}
|
||||
if (lstatIfExists(socketRoot)?.isSymbolicLink()) {
|
||||
const socketRootLstat = lstatIfExists(socketRoot);
|
||||
if (socketRootLstat?.isSymbolicLink()) {
|
||||
throw new Error("Coven covenHome must not be a symlink");
|
||||
}
|
||||
const socketRootStat = fs.statSync(socketRoot);
|
||||
validateSocketOwnerAndMode(socketRootStat, "Coven covenHome");
|
||||
|
||||
const socketStat = lstatIfExists(socketPath);
|
||||
if (socketStat?.isSymbolicLink()) {
|
||||
throw new Error("Coven socketPath must not be a symlink");
|
||||
}
|
||||
const resolvedSocketStat = fs.statSync(socketPath);
|
||||
if (!resolvedSocketStat.isSocket()) {
|
||||
throw new Error("Coven socketPath must be a Unix socket");
|
||||
}
|
||||
validateSocketOwnerAndMode(resolvedSocketStat, "Coven socketPath");
|
||||
|
||||
const realSocketRoot = fs.realpathSync.native(socketRoot);
|
||||
const realSocketDir = fs.realpathSync.native(path.dirname(socketPath));
|
||||
if (!pathIsInside(realSocketRoot, realSocketDir)) {
|
||||
@@ -115,14 +127,54 @@ function validateSocketPathForUse(socketPath: string, socketRoot: string | undef
|
||||
}
|
||||
}
|
||||
|
||||
function validateSocketOwnerAndMode(stat: fs.Stats, label: string): void {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
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`);
|
||||
}
|
||||
if ((stat.mode & 0o022) !== 0) {
|
||||
throw new Error(`${label} must not be group or world writable`);
|
||||
}
|
||||
}
|
||||
|
||||
function serializeRequestBody(body: unknown): { text: string; byteLength: number } {
|
||||
if (body === undefined) {
|
||||
return { text: "", byteLength: 0 };
|
||||
}
|
||||
const text = JSON.stringify(body) ?? "";
|
||||
const byteLength = Buffer.byteLength(text, "utf8");
|
||||
if (byteLength > MAX_REQUEST_BYTES) {
|
||||
throw new Error("Coven API request exceeded size limit");
|
||||
}
|
||||
return { text, byteLength };
|
||||
}
|
||||
|
||||
function errorToError(error: unknown): Error {
|
||||
return error instanceof Error ? error : new Error(String(error));
|
||||
}
|
||||
|
||||
function socketThatFailsWith(error: unknown): net.Socket {
|
||||
const socket = new net.Socket();
|
||||
queueMicrotask(() => socket.destroy(errorToError(error)));
|
||||
return socket;
|
||||
}
|
||||
|
||||
function requestOverSocket(options: RequestOptions): Promise<HttpResponse> {
|
||||
return new Promise((resolve, reject) => {
|
||||
if (options.signal?.aborted) {
|
||||
reject(options.signal.reason ?? new Error("request aborted"));
|
||||
return;
|
||||
}
|
||||
let requestBody = "";
|
||||
let requestBodyBytes = 0;
|
||||
try {
|
||||
validateSocketPathForUse(options.socketPath, options.socketRoot);
|
||||
const serialized = serializeRequestBody(options.body);
|
||||
requestBody = serialized.text;
|
||||
requestBodyBytes = serialized.byteLength;
|
||||
} catch (error) {
|
||||
reject(error);
|
||||
return;
|
||||
@@ -141,10 +193,16 @@ function requestOverSocket(options: RequestOptions): Promise<HttpResponse> {
|
||||
fn();
|
||||
};
|
||||
|
||||
const requestBody = options.body === undefined ? "" : JSON.stringify(options.body);
|
||||
const req = http.request(
|
||||
{
|
||||
socketPath: options.socketPath,
|
||||
createConnection: () => {
|
||||
try {
|
||||
validateSocketPathForUse(options.socketPath, options.socketRoot);
|
||||
return net.createConnection({ path: options.socketPath });
|
||||
} catch (error) {
|
||||
return socketThatFailsWith(error);
|
||||
}
|
||||
},
|
||||
method: options.method,
|
||||
path: options.path,
|
||||
headers: {
|
||||
@@ -153,7 +211,7 @@ function requestOverSocket(options: RequestOptions): Promise<HttpResponse> {
|
||||
...(requestBody
|
||||
? {
|
||||
"Content-Type": "application/json",
|
||||
"Content-Length": Buffer.byteLength(requestBody),
|
||||
"Content-Length": requestBodyBytes,
|
||||
}
|
||||
: {}),
|
||||
},
|
||||
|
||||
@@ -205,6 +205,65 @@ describe("CovenAcpRuntime", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("sanitizes daemon-controlled session fields in start status", async () => {
|
||||
const client = fakeClient({
|
||||
launchSession: vi.fn(async () =>
|
||||
session({
|
||||
id: "\u001b]0;spoof\u0007session-1\r",
|
||||
harness: "\u001b[31mcodex\u001b[0m",
|
||||
}),
|
||||
),
|
||||
});
|
||||
const runtime = new CovenAcpRuntime({ config, 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(events).toContainEqual(
|
||||
expect.objectContaining({ type: "status", text: "coven session session-1 started (codex)" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back without launching Coven when prompts exceed the Coven request limit", async () => {
|
||||
const fallback = fallbackRuntime();
|
||||
registerAcpRuntimeBackend({ id: "acpx", runtime: fallback });
|
||||
const client = fakeClient();
|
||||
const runtime = new CovenAcpRuntime({ config, client });
|
||||
const handle = await runtime.ensureSession({
|
||||
sessionKey: "agent:codex:test",
|
||||
agent: "codex",
|
||||
mode: "oneshot",
|
||||
cwd: workspaceDir,
|
||||
});
|
||||
|
||||
const events = await collect(
|
||||
runtime.runTurn({
|
||||
handle,
|
||||
text: "x".repeat(500_001),
|
||||
mode: "prompt",
|
||||
requestId: "req-1",
|
||||
}),
|
||||
);
|
||||
|
||||
expect(client.launchSession).not.toHaveBeenCalled();
|
||||
expect(events).toEqual([
|
||||
expect.objectContaining({ type: "text_delta", text: "direct fallback\n" }),
|
||||
expect.objectContaining({ type: "done", stopReason: "complete" }),
|
||||
]);
|
||||
});
|
||||
|
||||
it("ignores cwd embedded in runtimeSessionName when launching Coven sessions", async () => {
|
||||
const client = fakeClient();
|
||||
const runtime = new CovenAcpRuntime({ config, client });
|
||||
|
||||
@@ -32,8 +32,10 @@ const DEFAULT_HARNESSES: Record<string, string> = {
|
||||
opencode: "opencode",
|
||||
};
|
||||
const HEALTH_CHECK_TIMEOUT_MS = 5_000;
|
||||
const MAX_COVEN_PROMPT_BYTES = 500_000;
|
||||
const MAX_TRACKED_EVENT_IDS = 10_000;
|
||||
const MAX_RUNTIME_SESSION_NAME_BYTES = 2_048;
|
||||
const MAX_STATUS_FIELD_CHARS = 256;
|
||||
|
||||
type CovenRuntimeSessionState = {
|
||||
agent: string;
|
||||
@@ -150,6 +152,17 @@ function sanitizeStatusText(input: string): string {
|
||||
return sanitizeTerminalText(input).replace(/\s+/g, " ").trim();
|
||||
}
|
||||
|
||||
function sanitizeStatusField(input: string, fallback = "unknown"): string {
|
||||
return sanitizeStatusText(input).slice(0, MAX_STATUS_FIELD_CHARS) || fallback;
|
||||
}
|
||||
|
||||
function boundedCovenPrompt(input: string): string {
|
||||
if (Buffer.byteLength(input, "utf8") > MAX_COVEN_PROMPT_BYTES) {
|
||||
throw new Error("Coven prompt exceeded size limit");
|
||||
}
|
||||
return input;
|
||||
}
|
||||
|
||||
function normalizeStopReason(value: unknown): string {
|
||||
const normalized =
|
||||
typeof value === "string" ? sanitizeStatusText(value).toLowerCase() : "completed";
|
||||
@@ -172,8 +185,9 @@ function eventToRuntimeEvents(event: CovenEventRecord): AcpRuntimeEvent[] {
|
||||
return text ? [{ type: "text_delta", text, stream: "output", tag: "agent_message_chunk" }] : [];
|
||||
}
|
||||
if (event.kind === "exit") {
|
||||
const status = sanitizeStatusText(
|
||||
const status = sanitizeStatusField(
|
||||
typeof payload.status === "string" ? payload.status : "completed",
|
||||
"completed",
|
||||
);
|
||||
const exitCode = typeof payload.exitCode === "number" ? payload.exitCode : null;
|
||||
return [
|
||||
@@ -199,7 +213,7 @@ function sessionIsTerminal(session: CovenSessionRecord): boolean {
|
||||
}
|
||||
|
||||
function terminalStatusEvent(session: CovenSessionRecord): AcpRuntimeEvent {
|
||||
const status = sanitizeStatusText(session.status);
|
||||
const status = sanitizeStatusField(session.status, "completed");
|
||||
return {
|
||||
type: "status",
|
||||
text: `coven session ${status}${session.exitCode == null ? "" : ` exitCode=${session.exitCode}`}`,
|
||||
@@ -272,13 +286,14 @@ export class CovenAcpRuntime implements AcpRuntime {
|
||||
const cwd = this.resolveWorkspaceCwd(input.handle.cwd);
|
||||
let session: CovenSessionRecord;
|
||||
try {
|
||||
const prompt = boundedCovenPrompt(input.text);
|
||||
session = await this.client.launchSession(
|
||||
{
|
||||
projectRoot: this.config.workspaceDir,
|
||||
cwd,
|
||||
harness: this.resolveHarness(state.agent),
|
||||
prompt: input.text,
|
||||
title: titleFromPrompt(input.text),
|
||||
prompt,
|
||||
title: titleFromPrompt(prompt),
|
||||
},
|
||||
input.signal,
|
||||
);
|
||||
@@ -295,7 +310,7 @@ export class CovenAcpRuntime implements AcpRuntime {
|
||||
this.activeSessionIdsBySessionKey.set(input.handle.sessionKey, session.id);
|
||||
yield {
|
||||
type: "status",
|
||||
text: `coven session ${session.id} started (${session.harness})`,
|
||||
text: `coven session ${sanitizeStatusField(session.id)} started (${sanitizeStatusField(session.harness)})`,
|
||||
tag: "session_info_update",
|
||||
};
|
||||
|
||||
@@ -384,14 +399,17 @@ export class CovenAcpRuntime implements AcpRuntime {
|
||||
return { summary: "coven runtime ready" };
|
||||
}
|
||||
const session = await this.client.getSession(sessionId, input.signal);
|
||||
const status = sanitizeStatusField(session.status, "completed");
|
||||
const harness = sanitizeStatusField(session.harness);
|
||||
const title = sanitizeStatusField(session.title, "untitled");
|
||||
return {
|
||||
summary: `${sanitizeStatusText(session.status)} ${sanitizeStatusText(session.harness)} ${sanitizeStatusText(session.title)}`,
|
||||
summary: `${status} ${harness} ${title}`,
|
||||
backendSessionId: session.id,
|
||||
agentSessionId: session.id,
|
||||
details: {
|
||||
projectRoot: sanitizeStatusText(session.projectRoot),
|
||||
harness: sanitizeStatusText(session.harness),
|
||||
status: sanitizeStatusText(session.status),
|
||||
projectRoot: sanitizeStatusField(session.projectRoot),
|
||||
harness,
|
||||
status,
|
||||
exitCode: session.exitCode,
|
||||
},
|
||||
};
|
||||
@@ -534,6 +552,7 @@ export const __testing = {
|
||||
encodeRuntimeSessionName,
|
||||
eventToRuntimeEvents,
|
||||
normalizeStopReason,
|
||||
sanitizeStatusField,
|
||||
sanitizeTerminalText,
|
||||
titleFromPrompt,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user