From d60112287facfa4dfd01c14be8422a3f1ae0def6 Mon Sep 17 00:00:00 2001 From: Devin Robison Date: Tue, 24 Mar 2026 12:15:11 -0700 Subject: [PATCH] fix: validate agent workspace paths before writing identity files (#53882) * fix: validate agent workspace paths before writing identity files * Feedback updates and formatting fixes --- .../server-methods/agents-mutate.test.ts | 134 ++++++++++++++++-- src/gateway/server-methods/agents.ts | 105 ++++++++++++-- 2 files changed, 219 insertions(+), 20 deletions(-) diff --git a/src/gateway/server-methods/agents-mutate.test.ts b/src/gateway/server-methods/agents-mutate.test.ts index c29a7f1b828..df11219d612 100644 --- a/src/gateway/server-methods/agents-mutate.test.ts +++ b/src/gateway/server-methods/agents-mutate.test.ts @@ -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("../../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", () => { diff --git a/src/gateway/server-methods/agents.ts b/src/gateway/server-methods/agents.ts index 826fddb5994..88f84c216b8 100644 --- a/src/gateway/server-methods/agents.ts +++ b/src/gateway/server-methods/agents.ts @@ -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 { + 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 { + 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 }) => {