nyrn
/
openclaw
mirror of https://github.com/openclaw/openclaw.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: validate agent workspace paths before writing identity files (#53882) 

* fix: validate agent workspace paths before writing identity files

* Feedback updates and formatting fixes
This commit is contained in:
Devin Robison 2026-03-24 12:15:11 -07:00 committed by GitHub
parent 870c52aac7
commit d60112287f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 219 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

134
src/gateway/server-methods/agents-mutate.test.ts
View File

@ -1,5 +1,6 @@
import path from "node:path";
import { describe, expect, it, vi, beforeEach } from "vitest";
import { SafeOpenError } from "../../infra/fs-safe.js";
/* ------------------------------------------------------------------ */
/* Mocks */
@ -33,6 +34,7 @@ const mocks = vi.hoisted(() => ({
fsRealpath: vi.fn(async (p: string) => p),
fsReadlink: vi.fn(async () => ""),
fsOpen: vi.fn(async () => ({}) as unknown),
appendFileWithinRoot: vi.fn(async () => {}),
writeFileWithinRoot: vi.fn(async () => {}),
}));
@ -86,6 +88,7 @@ vi.mock("../../infra/fs-safe.js", async () => {
await vi.importActual<typeof import("../../infra/fs-safe.js")>("../../infra/fs-safe.js");
return {
...actual,
appendFileWithinRoot: mocks.appendFileWithinRoot,
writeFileWithinRoot: mocks.writeFileWithinRoot,
};
});
@ -262,6 +265,7 @@ describe("agents.create", () => {
mocks.loadConfigReturn = {};
mocks.findAgentEntryIndex.mockReturnValue(-1);
mocks.applyAgentConfig.mockImplementation((_cfg, _opts) => ({}));
mocks.appendFileWithinRoot.mockResolvedValue(undefined);
});
it("creates a new agent successfully", async () => {
@ -355,10 +359,13 @@ describe("agents.create", () => {
});
await promise;
expect(mocks.fsAppendFile).toHaveBeenCalledWith(
expect.stringContaining("IDENTITY.md"),
expect.stringContaining("- Name: Plain Agent"),
"utf-8",
expect(mocks.appendFileWithinRoot).toHaveBeenCalledWith(
expect.objectContaining({
rootDir: "/resolved/tmp/ws",
relativePath: "IDENTITY.md",
data: expect.stringContaining("- Name: Plain Agent"),
encoding: "utf8",
}),
);
});
@ -371,12 +378,60 @@ describe("agents.create", () => {
});
await promise;
expect(mocks.fsAppendFile).toHaveBeenCalledWith(
expect.stringContaining("IDENTITY.md"),
expect.stringMatching(/- Name: Fancy Agent[\s\S]*- Emoji: 🤖[\s\S]*- Avatar:/),
"utf-8",
expect(mocks.appendFileWithinRoot).toHaveBeenCalledWith(
expect.objectContaining({
rootDir: "/resolved/tmp/ws",
relativePath: "IDENTITY.md",
data: expect.stringMatching(/- Name: Fancy Agent[\s\S]*- Emoji: 🤖[\s\S]*- Avatar:/),
encoding: "utf8",
}),
);
});
it("rejects creating an agent when IDENTITY.md resolves outside the workspace", async () => {
const workspace = "/resolved/tmp/ws";
agentsTesting.setDepsForTests({
resolveAgentWorkspaceFilePath: async ({ name }) => ({
kind: "invalid",
requestPath: path.join(workspace, name),
reason: "path escapes workspace root",
}),
});
const { respond, promise } = makeCall("agents.create", {
name: "Unsafe Agent",
workspace: "/tmp/ws",
});
await promise;
expect(respond).toHaveBeenCalledWith(
false,
undefined,
expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }),
);
expect(mocks.writeConfigFile).not.toHaveBeenCalled();
expect(mocks.appendFileWithinRoot).not.toHaveBeenCalled();
});
it("does not persist config when IDENTITY.md append is rejected after preflight", async () => {
mocks.appendFileWithinRoot.mockRejectedValueOnce(
new SafeOpenError("path escapes workspace root", "path escapes workspace root"),
);
const { respond, promise } = makeCall("agents.create", {
name: "Append Reject Agent",
workspace: "/tmp/ws",
});
await promise;
expect(respond).toHaveBeenCalledWith(
false,
undefined,
expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }),
);
expect(mocks.appendFileWithinRoot).toHaveBeenCalledTimes(1);
expect(mocks.writeConfigFile).not.toHaveBeenCalled();
});
});
describe("agents.update", () => {
@ -385,6 +440,7 @@ describe("agents.update", () => {
mocks.loadConfigReturn = {};
mocks.findAgentEntryIndex.mockReturnValue(0);
mocks.applyAgentConfig.mockImplementation((_cfg, _opts) => ({}));
mocks.appendFileWithinRoot.mockResolvedValue(undefined);
});
it("updates an existing agent successfully", async () => {
@ -428,6 +484,68 @@ describe("agents.update", () => {
expect(mocks.ensureAgentWorkspace).not.toHaveBeenCalled();
});
it("appends avatar updates through appendFileWithinRoot", async () => {
const { promise } = makeCall("agents.update", {
agentId: "test-agent",
avatar: "https://example.com/avatar.png",
});
await promise;
expect(mocks.appendFileWithinRoot).toHaveBeenCalledWith(
expect.objectContaining({
rootDir: "/workspace/test-agent",
relativePath: "IDENTITY.md",
data: "\n- Avatar: https://example.com/avatar.png\n",
encoding: "utf8",
}),
);
});
it("rejects updating an agent when IDENTITY.md resolves outside the workspace", async () => {
const workspace = "/workspace/test-agent";
agentsTesting.setDepsForTests({
resolveAgentWorkspaceFilePath: async ({ name }) => ({
kind: "invalid",
requestPath: path.join(workspace, name),
reason: "path escapes workspace root",
}),
});
const { respond, promise } = makeCall("agents.update", {
agentId: "test-agent",
avatar: "evil.png",
});
await promise;
expect(respond).toHaveBeenCalledWith(
false,
undefined,
expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }),
);
expect(mocks.writeConfigFile).not.toHaveBeenCalled();
expect(mocks.appendFileWithinRoot).not.toHaveBeenCalled();
});
it("does not persist config when avatar append is rejected after preflight", async () => {
mocks.appendFileWithinRoot.mockRejectedValueOnce(
new SafeOpenError("path-mismatch", "path escapes workspace root"),
);
const { respond, promise } = makeCall("agents.update", {
agentId: "test-agent",
avatar: "https://example.com/avatar.png",
});
await promise;
expect(respond).toHaveBeenCalledWith(
false,
undefined,
expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }),
);
expect(mocks.appendFileWithinRoot).toHaveBeenCalledTimes(1);
expect(mocks.writeConfigFile).not.toHaveBeenCalled();
});
});
describe("agents.delete", () => {

105
src/gateway/server-methods/agents.ts
View File

@ -28,7 +28,12 @@ import {
import { loadConfig, writeConfigFile } from "../../config/config.js";
import { resolveSessionTranscriptsDirForAgent } from "../../config/sessions/paths.js";
import { sameFileIdentity } from "../../infra/file-identity.js";
import { SafeOpenError, readLocalFileSafely, writeFileWithinRoot } from "../../infra/fs-safe.js";
import {
appendFileWithinRoot,
SafeOpenError,
readLocalFileSafely,
writeFileWithinRoot,
} from "../../infra/fs-safe.js";
import { assertNoPathAliasEscape } from "../../infra/path-alias-guards.js";
import { isNotFoundPathError } from "../../infra/path-guards.js";
import { DEFAULT_AGENT_ID, normalizeAgentId } from "../../routing/session-key.js";
@ -65,6 +70,7 @@ const agentsHandlerDeps = {
isWorkspaceSetupCompleted,
readLocalFileSafely,
resolveAgentWorkspaceFilePath,
appendFileWithinRoot,
writeFileWithinRoot,
};
@ -74,6 +80,7 @@ export const __testing = {
isWorkspaceSetupCompleted: typeof isWorkspaceSetupCompleted;
readLocalFileSafely: typeof readLocalFileSafely;
resolveAgentWorkspaceFilePath: typeof resolveAgentWorkspaceFilePath;
appendFileWithinRoot: typeof appendFileWithinRoot;
writeFileWithinRoot: typeof writeFileWithinRoot;
}>,
) {
@ -83,6 +90,7 @@ export const __testing = {
agentsHandlerDeps.isWorkspaceSetupCompleted = isWorkspaceSetupCompleted;
agentsHandlerDeps.readLocalFileSafely = readLocalFileSafely;
agentsHandlerDeps.resolveAgentWorkspaceFilePath = resolveAgentWorkspaceFilePath;
agentsHandlerDeps.appendFileWithinRoot = appendFileWithinRoot;
agentsHandlerDeps.writeFileWithinRoot = writeFileWithinRoot;
},
};
@ -481,6 +489,39 @@ function respondWorkspaceFileMissing(params: {
);
}
async function ensureWorkspaceFileReadyOrRespond(params: {
respond: RespondFn;
workspaceDir: string;
name: string;
}): Promise<boolean> {
await fs.mkdir(params.workspaceDir, { recursive: true });
const resolvedPath = await resolveWorkspaceFilePathOrRespond(params);
return resolvedPath !== undefined;
}
async function appendWorkspaceFileOrRespond(params: {
respond: RespondFn;
workspaceDir: string;
name: string;
content: string;
}): Promise<boolean> {
try {
await agentsHandlerDeps.appendFileWithinRoot({
rootDir: params.workspaceDir,
relativePath: params.name,
data: params.content,
encoding: "utf8",
});
} catch (err) {
if (err instanceof SafeOpenError) {
respondWorkspaceFileUnsafe(params.respond, params.name);
return false;
}
throw err;
}
return true;
}
export const agentsHandlers: GatewayRequestHandlers = {
"agents.list": ({ params, respond }) => {
if (!validateAgentsListParams(params)) {
@ -553,13 +594,10 @@ export const agentsHandlers: GatewayRequestHandlers = {
await ensureAgentWorkspace({ dir: workspaceDir, ensureBootstrapFiles: !skipBootstrap });
await fs.mkdir(resolveSessionTranscriptsDirForAgent(agentId), { recursive: true });
await writeConfigFile(nextConfig);
// Always write Name to IDENTITY.md; optionally include emoji/avatar.
const safeName = sanitizeIdentityLine(rawName);
const emoji = resolveOptionalStringParam(params.emoji);
const avatar = resolveOptionalStringParam(params.avatar);
const identityPath = path.join(workspaceDir, DEFAULT_IDENTITY_FILENAME);
const lines = [
"",
`- Name: ${safeName}`,
@ -567,7 +605,28 @@ export const agentsHandlers: GatewayRequestHandlers = {
...(avatar ? [`- Avatar: ${sanitizeIdentityLine(avatar)}`] : []),
"",
];
await fs.appendFile(identityPath, lines.join("\n"), "utf-8");
if (
!(await ensureWorkspaceFileReadyOrRespond({
respond,
workspaceDir,
name: DEFAULT_IDENTITY_FILENAME,
}))
) {
return;
}
if (
!(await appendWorkspaceFileOrRespond({
respond,
workspaceDir,
name: DEFAULT_IDENTITY_FILENAME,
content: lines.join("\n"),
}))
) {
return;
}
await writeConfigFile(nextConfig);
respond(true, { ok: true, agentId, name: rawName, workspace: workspaceDir }, undefined);
},
@ -601,20 +660,42 @@ export const agentsHandlers: GatewayRequestHandlers = {
...(model ? { model } : {}),
});
await writeConfigFile(nextConfig);
if (workspaceDir) {
const skipBootstrap = Boolean(nextConfig.agents?.defaults?.skipBootstrap);
await ensureAgentWorkspace({ dir: workspaceDir, ensureBootstrapFiles: !skipBootstrap });
}
if (avatar) {
const workspace = workspaceDir ?? resolveAgentWorkspaceDir(nextConfig, agentId);
await fs.mkdir(workspace, { recursive: true });
const identityPath = path.join(workspace, DEFAULT_IDENTITY_FILENAME);
await fs.appendFile(identityPath, `\n- Avatar: ${sanitizeIdentityLine(avatar)}\n`, "utf-8");
const identityWorkspaceDir = avatar ? resolveAgentWorkspaceDir(nextConfig, agentId) : undefined;
if (
identityWorkspaceDir &&
!(await ensureWorkspaceFileReadyOrRespond({
respond,
workspaceDir: identityWorkspaceDir,
name: DEFAULT_IDENTITY_FILENAME,
}))
) {
return;
}
if (avatar) {
if (!identityWorkspaceDir) {
respondWorkspaceFileUnsafe(respond, DEFAULT_IDENTITY_FILENAME);
return;
}
if (
!(await appendWorkspaceFileOrRespond({
respond,
workspaceDir: identityWorkspaceDir,
name: DEFAULT_IDENTITY_FILENAME,
content: `\n- Avatar: ${sanitizeIdentityLine(avatar)}\n`,
}))
) {
return;
}
}
await writeConfigFile(nextConfig);
respond(true, { ok: true, agentId }, undefined);
},
"agents.delete": async ({ params, respond }) => {