diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ccad11ee76..c5fd66d0f7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Docs: https://docs.openclaw.ai - Onboarding/channels: normalize channel setup metadata before discovery and validation so malformed or mixed-shape channel plugin metadata no longer breaks setup and onboarding channel lists. (#66706) Thanks @darkamenosa. - Slack/native commands: fix option menus for slash commands such as `/verbose` when Slack renders native buttons by giving each button a unique action ID while still routing them through the shared `openclaw_cmdarg*` listener. Thanks @Wangmerlyn. - Feishu/webhook: harden the webhook transport and card-action replay guards to fail closed on missing `encryptKey` and blank callback tokens — refuse to start the webhook transport without an `encryptKey`, reject unsigned requests when no key is present instead of accepting them, and drop blank card-action tokens before the dedupe claim and dispatcher. Defense-in-depth over the already-closed monitor-account layer. (#66707) Thanks @eleqtrizit. +- Agents/workspace files: route `agents.files.get`, `agents.files.set`, and workspace listing through the shared `fs-safe` helpers (`openFileWithinRoot`/`readFileWithinRoot`/`writeFileWithinRoot`), reject symlink aliases for allowlisted agent files, and have `fs-safe` resolve opened-file real paths from the file descriptor before falling back to path-based `realpath` so a symlink swap between `open` and `realpath` can no longer redirect the validated path off the intended inode. (#66636) Thanks @eleqtrizit. ## 2026.4.14 diff --git a/src/gateway/server-methods/agents-mutate.test.ts b/src/gateway/server-methods/agents-mutate.test.ts index d96825823b4..c46da5a3f8e 100644 --- a/src/gateway/server-methods/agents-mutate.test.ts +++ b/src/gateway/server-methods/agents-mutate.test.ts @@ -1,5 +1,5 @@ -import path from "node:path"; import { describe, expect, it, vi, beforeEach } from "vitest"; +import { SafeOpenError } from "../../infra/fs-safe.js"; /* ------------------------------------------------------------------ */ /* Mocks */ /* ------------------------------------------------------------------ */ @@ -499,9 +499,8 @@ describe("agents.create", () => { }); it("does not persist config when IDENTITY.md write fails with SafeOpenError", async () => { - const { SafeOpenError: SOE } = await import("../../infra/fs-safe.js"); mocks.writeFileWithinRoot.mockRejectedValueOnce( - new SOE("path-mismatch", "path escapes workspace root"), + new SafeOpenError("path-mismatch", "path escapes workspace root"), ); const { respond, promise } = makeCall("agents.create", { @@ -520,24 +519,7 @@ describe("agents.create", () => { it("does not persist config when IDENTITY.md read fails", async () => { agentsTesting.setDepsForTests({ - resolveAgentWorkspaceFilePath: async ({ workspaceDir, name }) => { - const ioPath = `${workspaceDir}/${name}`; - if (workspaceDir === "/resolved/tmp/ws") { - return { - kind: "ready", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - } - return { - kind: "missing", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - }, - readLocalFileSafely: async () => { + readFileWithinRoot: async () => { throw createErrnoError("EACCES"); }, }); @@ -556,6 +538,50 @@ describe("agents.create", () => { expect(mocks.writeFileWithinRoot).not.toHaveBeenCalled(); }); + it("treats unsafe IDENTITY.md reads as invalid create requests", async () => { + agentsTesting.setDepsForTests({ + readFileWithinRoot: async () => { + throw new SafeOpenError("invalid-path", "path is not a regular file under root"); + }, + }); + + const { respond, promise } = makeCall("agents.create", { + name: "Unsafe Identity Read", + workspace: "/tmp/ws", + }); + await promise; + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: expect.stringContaining('unsafe workspace file "IDENTITY.md"'), + }), + ); + expect(mocks.writeConfigFile).not.toHaveBeenCalled(); + expect(mocks.writeFileWithinRoot).not.toHaveBeenCalled(); + }); + + it("uses non-blocking reads for IDENTITY.md during agents.create", async () => { + const readFileWithinRoot = vi.fn(async () => { + throw new SafeOpenError("not-found", "file not found"); + }); + agentsTesting.setDepsForTests({ readFileWithinRoot }); + + const { promise } = makeCall("agents.create", { + name: "NB Agent", + workspace: "/tmp/ws", + }); + await promise; + + expect(readFileWithinRoot).toHaveBeenCalledWith( + expect.objectContaining({ + relativePath: "IDENTITY.md", + nonBlockingRead: true, + }), + ); + }); + it("passes model to applyAgentConfig when provided", async () => { const { respond, promise } = makeCall("agents.create", { name: "Model Agent", @@ -728,27 +754,8 @@ describe("agents.update", () => { identityPathCreated: true, }); agentsTesting.setDepsForTests({ - resolveAgentWorkspaceFilePath: async ({ workspaceDir, name }) => { - const ioPath = `${workspaceDir}/${name}`; - if ( - workspaceDir === "/workspace/test-agent" || - workspaceDir === "/resolved/new/workspace" - ) { - return { - kind: "ready", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - } - return { - kind: "missing", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - }, - readLocalFileSafely: async ({ filePath }) => { + readFileWithinRoot: async ({ rootDir, relativePath }) => { + const filePath = `${rootDir}/${relativePath}`; if (filePath === "/workspace/test-agent/IDENTITY.md") { return { buffer: Buffer.from( @@ -825,27 +832,8 @@ describe("agents.update", () => { identityPathCreated: false, }); agentsTesting.setDepsForTests({ - resolveAgentWorkspaceFilePath: async ({ workspaceDir, name }) => { - const ioPath = `${workspaceDir}/${name}`; - if ( - workspaceDir === "/workspace/test-agent" || - workspaceDir === "/resolved/new/workspace" - ) { - return { - kind: "ready", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - } - return { - kind: "missing", - requestPath: ioPath, - ioPath, - workspaceReal: workspaceDir, - }; - }, - readLocalFileSafely: async ({ filePath }) => { + readFileWithinRoot: async ({ rootDir, relativePath }) => { + const filePath = `${rootDir}/${relativePath}`; if (filePath === "/workspace/test-agent/IDENTITY.md") { return { buffer: Buffer.from( @@ -915,9 +903,8 @@ describe("agents.update", () => { }); it("does not persist config when IDENTITY.md write fails on update", async () => { - const { SafeOpenError: SOE } = await import("../../infra/fs-safe.js"); mocks.writeFileWithinRoot.mockRejectedValueOnce( - new SOE("path-mismatch", "path escapes workspace root"), + new SafeOpenError("path-mismatch", "path escapes workspace root"), ); const { respond, promise } = makeCall("agents.update", { @@ -934,6 +921,50 @@ describe("agents.update", () => { ); expect(mocks.writeConfigFile).not.toHaveBeenCalled(); }); + + it("treats unsafe IDENTITY.md reads as invalid update requests", async () => { + agentsTesting.setDepsForTests({ + readFileWithinRoot: async () => { + throw new SafeOpenError("invalid-path", "path is not a regular file under root"); + }, + }); + + const { respond, promise } = makeCall("agents.update", { + agentId: "test-agent", + avatar: "https://example.com/unsafe.png", + }); + await promise; + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: expect.stringContaining('unsafe workspace file "IDENTITY.md"'), + }), + ); + expect(mocks.writeConfigFile).not.toHaveBeenCalled(); + expect(mocks.writeFileWithinRoot).not.toHaveBeenCalled(); + }); + + it("uses non-blocking reads for IDENTITY.md during agents.update", async () => { + const readFileWithinRoot = vi.fn(async () => { + throw new SafeOpenError("not-found", "file not found"); + }); + agentsTesting.setDepsForTests({ readFileWithinRoot }); + + const { promise } = makeCall("agents.update", { + agentId: "test-agent", + name: "Updated NB", + }); + await promise; + + expect(readFileWithinRoot).toHaveBeenCalledWith( + expect.objectContaining({ + relativePath: "IDENTITY.md", + nonBlockingRead: true, + }), + ); + }); }); describe("agents.delete", () => { @@ -1044,6 +1075,40 @@ describe("agents.files.list", () => { const names = await listAgentFileNames(); expect(names).toContain("BOOTSTRAP.md"); }); + + it("reports unreadable workspace files as present in list responses", async () => { + const openFileWithinRoot = vi.fn(async () => { + throw createErrnoError("EACCES"); + }); + agentsTesting.setDepsForTests({ openFileWithinRoot }); + mocks.fsLstat.mockImplementation(async (...args: unknown[]) => { + if (args[0] === "/workspace/main/AGENTS.md") { + return makeFileStat({ size: 17, mtimeMs: 4567 }); + } + throw createEnoentError(); + }); + mocks.fsStat.mockImplementation(async (...args: unknown[]) => { + if (args[0] === "/workspace/main/AGENTS.md") { + return makeFileStat({ size: 17, mtimeMs: 4567 }); + } + throw createEnoentError(); + }); + + const { respond, promise } = makeCall("agents.files.list", { agentId: "main" }); + await promise; + + const [, result] = respond.mock.calls[0] ?? []; + const files = (result as { files: Array<{ name: string; missing: boolean; size?: number }> }) + .files; + expect(files).toContainEqual( + expect.objectContaining({ + name: "AGENTS.md", + missing: false, + size: 17, + }), + ); + expect(openFileWithinRoot).not.toHaveBeenCalled(); + }); }); describe("agents.files.get/set symlink safety", () => { @@ -1058,14 +1123,32 @@ describe("agents.files.get/set symlink safety", () => { }); function mockWorkspaceEscapeSymlink() { - const workspace = "/workspace/test-agent"; + const safeOpenError = new SafeOpenError("invalid-path", "path escapes workspace root"); agentsTesting.setDepsForTests({ - resolveAgentWorkspaceFilePath: async ({ name }) => ({ - kind: "invalid", - requestPath: path.join(workspace, name), - reason: "path escapes workspace root", - }), + openFileWithinRoot: async () => { + throw safeOpenError; + }, + readFileWithinRoot: async () => { + throw safeOpenError; + }, }); + mocks.writeFileWithinRoot.mockRejectedValue(safeOpenError); + } + + function mockInWorkspaceSymlinkAlias() { + const safeOpenError = new SafeOpenError( + "invalid-path", + "path is not a regular file under root", + ); + agentsTesting.setDepsForTests({ + openFileWithinRoot: async () => { + throw safeOpenError; + }, + readFileWithinRoot: async () => { + throw safeOpenError; + }, + }); + mocks.writeFileWithinRoot.mockRejectedValue(safeOpenError); } it.each([ @@ -1082,83 +1165,25 @@ describe("agents.files.get/set symlink safety", () => { }, ); - it("allows in-workspace symlink reads and writes through symlink aliases", async () => { - const workspace = "/workspace/test-agent"; - const target = path.resolve(workspace, "policies", "AGENTS.md"); - const targetStat = makeFileStat({ size: 7, mtimeMs: 1700, dev: 9, ino: 42 }); - - agentsTesting.setDepsForTests({ - readLocalFileSafely: async () => ({ - buffer: Buffer.from("inside\n"), - realPath: target, - stat: targetStat, - }), - resolveAgentWorkspaceFilePath: async ({ name }) => ({ - kind: "ready", - requestPath: path.join(workspace, name), - ioPath: target, - workspaceReal: workspace, - }), - }); - mocks.fsLstat.mockImplementation(async (...args: unknown[]) => { - const p = typeof args[0] === "string" ? args[0] : ""; - if (p === target) { - return targetStat; - } - throw createEnoentError(); - }); - mocks.fsStat.mockImplementation(async (...args: unknown[]) => { - const p = typeof args[0] === "string" ? args[0] : ""; - if (p === target) { - return targetStat; - } - throw createEnoentError(); - }); - - const getCall = makeCall("agents.files.get", { agentId: "main", name: "AGENTS.md" }); - await getCall.promise; - expect(getCall.respond).toHaveBeenCalledWith( - true, - expect.objectContaining({ - file: expect.objectContaining({ missing: false, content: "inside\n" }), - }), - undefined, - ); - - const setCall = makeCall("agents.files.set", { - agentId: "main", - name: "AGENTS.md", - content: "updated\n", - }); - await setCall.promise; - expect(setCall.respond).toHaveBeenCalledWith( - true, - expect.objectContaining({ - file: expect.objectContaining({ - missing: false, - content: "updated\n", - }), - }), - undefined, - ); - }); + it.each(["agents.files.get", "agents.files.set"] as const)( + "rejects %s when allowlisted file is an in-workspace symlink alias", + async (method) => { + mockInWorkspaceSymlinkAlias(); + await expectUnsafeWorkspaceFile(method); + }, + ); function mockHardlinkedWorkspaceAlias() { - const workspace = "/workspace/test-agent"; - const candidate = path.resolve(workspace, "AGENTS.md"); - mocks.fsRealpath.mockImplementation(async (p: string) => { - if (p === workspace) { - return workspace; - } - return p; - }); - mocks.fsLstat.mockImplementation(async (...args: unknown[]) => { - const p = typeof args[0] === "string" ? args[0] : ""; - if (p === candidate) { - return makeFileStat({ nlink: 2 }); - } - throw createEnoentError(); + const safeOpenError = new SafeOpenError("invalid-path", "hardlinked path not allowed"); + agentsTesting.setDepsForTests({ + openFileWithinRoot: async () => { + throw safeOpenError; + }, + readFileWithinRoot: async () => { + throw safeOpenError; + }, }); + mocks.writeFileWithinRoot.mockRejectedValue(safeOpenError); } it.each([ @@ -1174,4 +1199,38 @@ describe("agents.files.get/set symlink safety", () => { } }, ); + + it("uses non-blocking safe reads for agents.files.get", async () => { + const readFileWithinRoot = vi.fn(async () => ({ + buffer: Buffer.from("hello"), + realPath: "/workspace/test-agent/AGENTS.md", + stat: makeFileStat({ size: 5 }), + })); + agentsTesting.setDepsForTests({ readFileWithinRoot }); + + const { respond, promise } = makeCall("agents.files.get", { + agentId: "main", + name: "AGENTS.md", + }); + await promise; + + expect(readFileWithinRoot).toHaveBeenCalledWith( + expect.objectContaining({ + rootDir: "/workspace/test-agent", + relativePath: "AGENTS.md", + rejectHardlinks: true, + nonBlockingRead: true, + }), + ); + expect(respond).toHaveBeenCalledWith( + true, + expect.objectContaining({ + file: expect.objectContaining({ + name: "AGENTS.md", + content: "hello", + }), + }), + undefined, + ); + }); }); diff --git a/src/gateway/server-methods/agents.ts b/src/gateway/server-methods/agents.ts index 0c72833dd84..d905876538e 100644 --- a/src/gateway/server-methods/agents.ts +++ b/src/gateway/server-methods/agents.ts @@ -31,9 +31,12 @@ import { resolveSessionTranscriptsDirForAgent } from "../../config/sessions/path import type { IdentityConfig } from "../../config/types.base.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { sameFileIdentity } from "../../infra/file-identity.js"; -import { SafeOpenError, readLocalFileSafely, writeFileWithinRoot } from "../../infra/fs-safe.js"; -import { assertNoPathAliasEscape } from "../../infra/path-alias-guards.js"; -import { isNotFoundPathError } from "../../infra/path-guards.js"; +import { + openFileWithinRoot, + readFileWithinRoot, + SafeOpenError, + writeFileWithinRoot, +} from "../../infra/fs-safe.js"; import { movePathToTrash } from "../../plugin-sdk/browser-maintenance.js"; import { DEFAULT_AGENT_ID, normalizeAgentId } from "../../routing/session-key.js"; import { resolveUserPath } from "../../utils.js"; @@ -67,8 +70,8 @@ const BOOTSTRAP_FILE_NAMES_POST_ONBOARDING = BOOTSTRAP_FILE_NAMES.filter( const agentsHandlerDeps = { isWorkspaceSetupCompleted, - readLocalFileSafely, - resolveAgentWorkspaceFilePath, + openFileWithinRoot, + readFileWithinRoot, writeFileWithinRoot, }; @@ -76,8 +79,8 @@ export const __testing = { setDepsForTests( overrides: Partial<{ isWorkspaceSetupCompleted: typeof isWorkspaceSetupCompleted; - readLocalFileSafely: typeof readLocalFileSafely; - resolveAgentWorkspaceFilePath: typeof resolveAgentWorkspaceFilePath; + openFileWithinRoot: typeof openFileWithinRoot; + readFileWithinRoot: typeof readFileWithinRoot; writeFileWithinRoot: typeof writeFileWithinRoot; }>, ) { @@ -85,8 +88,8 @@ export const __testing = { }, resetDepsForTests() { agentsHandlerDeps.isWorkspaceSetupCompleted = isWorkspaceSetupCompleted; - agentsHandlerDeps.readLocalFileSafely = readLocalFileSafely; - agentsHandlerDeps.resolveAgentWorkspaceFilePath = resolveAgentWorkspaceFilePath; + agentsHandlerDeps.openFileWithinRoot = openFileWithinRoot; + agentsHandlerDeps.readFileWithinRoot = readFileWithinRoot; agentsHandlerDeps.writeFileWithinRoot = writeFileWithinRoot; }, }; @@ -131,166 +134,40 @@ type FileMeta = { updatedAtMs: number; }; -type ResolvedAgentWorkspaceFilePath = - | { - kind: "ready"; - requestPath: string; - ioPath: string; - workspaceReal: string; - } - | { - kind: "missing"; - requestPath: string; - ioPath: string; - workspaceReal: string; - } - | { - kind: "invalid"; - requestPath: string; - reason: string; - }; - -type ResolvedWorkspaceFilePath = Exclude; - -function resolveNotFoundWorkspaceFilePathResult(params: { - error: unknown; - allowMissing: boolean; - requestPath: string; - ioPath: string; - workspaceReal: string; -}): Extract | undefined { - if (!isNotFoundPathError(params.error)) { - return undefined; - } - if (params.allowMissing) { - return { - kind: "missing", - requestPath: params.requestPath, - ioPath: params.ioPath, - workspaceReal: params.workspaceReal, - }; - } - return { kind: "invalid", requestPath: params.requestPath, reason: "file not found" }; +function isPathInsideDirectory(rootDir: string, candidatePath: string): boolean { + const relative = path.relative(rootDir, candidatePath); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); } -function resolveWorkspaceFilePathResultOrThrow(params: { - error: unknown; - allowMissing: boolean; - requestPath: string; - ioPath: string; - workspaceReal: string; -}): Extract { - const notFoundResult = resolveNotFoundWorkspaceFilePathResult(params); - if (notFoundResult) { - return notFoundResult; - } - throw params.error; -} - -async function resolveWorkspaceRealPath(workspaceDir: string): Promise { +async function statWorkspaceFileSafely( + workspaceDir: string, + name: string, +): Promise { try { - return await fs.realpath(workspaceDir); - } catch { - return path.resolve(workspaceDir); - } -} - -async function resolveAgentWorkspaceFilePath(params: { - workspaceDir: string; - name: string; - allowMissing: boolean; -}): Promise { - const requestPath = path.join(params.workspaceDir, params.name); - const workspaceReal = await resolveWorkspaceRealPath(params.workspaceDir); - const candidatePath = path.resolve(workspaceReal, params.name); - - try { - await assertNoPathAliasEscape({ - absolutePath: candidatePath, - rootPath: workspaceReal, - boundaryLabel: "workspace root", - }); - } catch (error) { - return { - kind: "invalid", - requestPath, - reason: error instanceof Error ? error.message : "path escapes workspace root", - }; - } - - const notFoundContext = { - allowMissing: params.allowMissing, - requestPath, - workspaceReal, - } as const; - - let candidateLstat: Awaited>; - try { - candidateLstat = await fs.lstat(candidatePath); - } catch (err) { - return resolveWorkspaceFilePathResultOrThrow({ - error: err, - ...notFoundContext, - ioPath: candidatePath, - }); - } - - if (candidateLstat.isSymbolicLink()) { - let targetReal: string; - try { - targetReal = await fs.realpath(candidatePath); - } catch (err) { - return resolveWorkspaceFilePathResultOrThrow({ - error: err, - ...notFoundContext, - ioPath: candidatePath, - }); - } - let targetStat: Awaited>; - try { - targetStat = await fs.stat(targetReal); - } catch (err) { - return resolveWorkspaceFilePathResultOrThrow({ - error: err, - ...notFoundContext, - ioPath: targetReal, - }); - } - if (!targetStat.isFile()) { - return { kind: "invalid", requestPath, reason: "path is not a regular file" }; - } - if (targetStat.nlink > 1) { - return { kind: "invalid", requestPath, reason: "hardlinked file path not allowed" }; - } - return { kind: "ready", requestPath, ioPath: targetReal, workspaceReal }; - } - - if (!candidateLstat.isFile()) { - return { kind: "invalid", requestPath, reason: "path is not a regular file" }; - } - if (candidateLstat.nlink > 1) { - return { kind: "invalid", requestPath, reason: "hardlinked file path not allowed" }; - } - - const targetReal = await fs.realpath(candidatePath).catch(() => candidatePath); - return { kind: "ready", requestPath, ioPath: targetReal, workspaceReal }; -} - -async function statFileSafely(filePath: string): Promise { - try { - const [stat, lstat] = await Promise.all([fs.stat(filePath), fs.lstat(filePath)]); - if (lstat.isSymbolicLink() || !stat.isFile()) { + const workspaceReal = await fs.realpath(workspaceDir); + const candidatePath = path.resolve(workspaceReal, name); + if (!isPathInsideDirectory(workspaceReal, candidatePath)) { return null; } - if (stat.nlink > 1) { + + const pathStat = await fs.lstat(candidatePath); + if (!pathStat.isFile() || pathStat.nlink > 1) { return null; } - if (!sameFileIdentity(stat, lstat)) { + + const realPath = await fs.realpath(candidatePath); + if (!isPathInsideDirectory(workspaceReal, realPath)) { return null; } + + const realStat = await fs.stat(realPath); + if (!realStat.isFile() || realStat.nlink > 1 || !sameFileIdentity(pathStat, realStat)) { + return null; + } + return { - size: stat.size, - updatedAtMs: Math.floor(stat.mtimeMs), + size: realStat.size, + updatedAtMs: Math.floor(realStat.mtimeMs), }; } catch { return null; @@ -310,18 +187,8 @@ async function listAgentFiles(workspaceDir: string, options?: { hideBootstrap?: ? BOOTSTRAP_FILE_NAMES_POST_ONBOARDING : BOOTSTRAP_FILE_NAMES; for (const name of bootstrapFileNames) { - const resolved = await resolveAgentWorkspaceFilePath({ - workspaceDir, - name, - allowMissing: true, - }); - const filePath = resolved.requestPath; - const meta = - resolved.kind === "ready" - ? await statFileSafely(resolved.ioPath) - : resolved.kind === "missing" - ? null - : null; + const filePath = path.join(workspaceDir, name); + const meta = await statWorkspaceFileSafely(workspaceDir, name); if (meta) { files.push({ name, @@ -335,33 +202,21 @@ async function listAgentFiles(workspaceDir: string, options?: { hideBootstrap?: } } - const primaryResolved = await resolveAgentWorkspaceFilePath({ - workspaceDir, - name: DEFAULT_MEMORY_FILENAME, - allowMissing: true, - }); - const primaryMeta = - primaryResolved.kind === "ready" ? await statFileSafely(primaryResolved.ioPath) : null; + const primaryMeta = await statWorkspaceFileSafely(workspaceDir, DEFAULT_MEMORY_FILENAME); if (primaryMeta) { files.push({ name: DEFAULT_MEMORY_FILENAME, - path: primaryResolved.requestPath, + path: path.join(workspaceDir, DEFAULT_MEMORY_FILENAME), missing: false, size: primaryMeta.size, updatedAtMs: primaryMeta.updatedAtMs, }); } else { - const altMemoryResolved = await resolveAgentWorkspaceFilePath({ - workspaceDir, - name: DEFAULT_MEMORY_ALT_FILENAME, - allowMissing: true, - }); - const altMeta = - altMemoryResolved.kind === "ready" ? await statFileSafely(altMemoryResolved.ioPath) : null; + const altMeta = await statWorkspaceFileSafely(workspaceDir, DEFAULT_MEMORY_ALT_FILENAME); if (altMeta) { files.push({ name: DEFAULT_MEMORY_ALT_FILENAME, - path: altMemoryResolved.requestPath, + path: path.join(workspaceDir, DEFAULT_MEMORY_ALT_FILENAME), missing: false, size: altMeta.size, updatedAtMs: altMeta.updatedAtMs, @@ -369,7 +224,7 @@ async function listAgentFiles(workspaceDir: string, options?: { hideBootstrap?: } else { files.push({ name: DEFAULT_MEMORY_FILENAME, - path: primaryResolved.requestPath, + path: path.join(workspaceDir, DEFAULT_MEMORY_FILENAME), missing: true, }); } @@ -434,31 +289,6 @@ async function moveToTrashBestEffort(pathname: string): Promise { } } -function respondWorkspaceFileInvalid(respond: RespondFn, name: string, reason: string): void { - respond( - false, - undefined, - errorShape(ErrorCodes.INVALID_REQUEST, `unsafe workspace file "${name}" (${reason})`), - ); -} - -async function resolveWorkspaceFilePathOrRespond(params: { - respond: RespondFn; - workspaceDir: string; - name: string; -}): Promise { - const resolvedPath = await agentsHandlerDeps.resolveAgentWorkspaceFilePath({ - workspaceDir: params.workspaceDir, - name: params.name, - allowMissing: true, - }); - if (resolvedPath.kind === "invalid") { - respondWorkspaceFileInvalid(params.respond, params.name, resolvedPath.reason); - return undefined; - } - return resolvedPath; -} - function respondWorkspaceFileUnsafe(respond: RespondFn, name: string): void { respond( false, @@ -492,27 +322,10 @@ async function writeWorkspaceFileOrRespond(params: { content: string; }): Promise { await fs.mkdir(params.workspaceDir, { recursive: true }); - const resolvedPath = await resolveWorkspaceFilePathOrRespond({ - respond: params.respond, - workspaceDir: params.workspaceDir, - name: params.name, - }); - if (!resolvedPath) { - return false; - } - const relativeWritePath = path.relative(resolvedPath.workspaceReal, resolvedPath.ioPath); - if ( - !relativeWritePath || - relativeWritePath.startsWith("..") || - path.isAbsolute(relativeWritePath) - ) { - respondWorkspaceFileUnsafe(params.respond, params.name); - return false; - } try { await agentsHandlerDeps.writeFileWithinRoot({ - rootDir: resolvedPath.workspaceReal, - relativePath: relativeWritePath, + rootDir: params.workspaceDir, + relativePath: params.name, data: params.content, encoding: "utf8", }); @@ -548,16 +361,13 @@ async function readWorkspaceFileContent( workspaceDir: string, name: string, ): Promise { - const resolvedPath = await agentsHandlerDeps.resolveAgentWorkspaceFilePath({ - workspaceDir, - name, - allowMissing: true, - }); - if (resolvedPath.kind !== "ready") { - return undefined; - } try { - const safeRead = await agentsHandlerDeps.readLocalFileSafely({ filePath: resolvedPath.ioPath }); + const safeRead = await agentsHandlerDeps.readFileWithinRoot({ + rootDir: workspaceDir, + relativePath: name, + rejectHardlinks: true, + nonBlockingRead: true, + }); return safeRead.buffer.toString("utf-8"); } catch (err) { if (err instanceof SafeOpenError && err.code === "not-found") { @@ -595,6 +405,24 @@ async function buildIdentityMarkdownForWrite(params: { return mergeIdentityMarkdownContent(baseContent, params.identity); } +async function buildIdentityMarkdownOrRespondUnsafe(params: { + respond: RespondFn; + workspaceDir: string; + identity: IdentityConfig; + fallbackWorkspaceDir?: string; + preferFallbackWorkspaceContent?: boolean; +}): Promise { + try { + return await buildIdentityMarkdownForWrite(params); + } catch (err) { + if (err instanceof SafeOpenError) { + respondWorkspaceFileUnsafe(params.respond, DEFAULT_IDENTITY_FILENAME); + return null; + } + throw err; + } +} + export const agentsHandlers: GatewayRequestHandlers = { "agents.list": ({ params, respond }) => { if (!validateAgentsListParams(params)) { @@ -682,10 +510,14 @@ export const agentsHandlers: GatewayRequestHandlers = { const persistedIdentity = normalizeIdentityForFile(resolveAgentIdentity(nextConfig, agentId)); if (persistedIdentity) { - const identityContent = await buildIdentityMarkdownForWrite({ + const identityContent = await buildIdentityMarkdownOrRespondUnsafe({ + respond, workspaceDir, identity: persistedIdentity, }); + if (identityContent === null) { + return; + } if ( !(await writeWorkspaceFileOrRespond({ respond, @@ -762,13 +594,17 @@ export const agentsHandlers: GatewayRequestHandlers = { workspaceDir && identityWorkspaceDir !== previousWorkspaceDir ? previousWorkspaceDir : undefined; - const identityContent = await buildIdentityMarkdownForWrite({ + const identityContent = await buildIdentityMarkdownOrRespondUnsafe({ + respond, workspaceDir: identityWorkspaceDir, identity: persistedIdentity, fallbackWorkspaceDir, preferFallbackWorkspaceContent: Boolean(fallbackWorkspaceDir) && ensuredWorkspace?.identityPathCreated === true, }); + if (identityContent === null) { + return; + } if ( !(await writeWorkspaceFileOrRespond({ respond, @@ -865,28 +701,24 @@ export const agentsHandlers: GatewayRequestHandlers = { } const { agentId, workspaceDir, name } = resolved; const filePath = path.join(workspaceDir, name); - const resolvedPath = await resolveWorkspaceFilePathOrRespond({ - respond, - workspaceDir, - name, - }); - if (!resolvedPath) { - return; - } - if (resolvedPath.kind === "missing") { - respondWorkspaceFileMissing({ respond, agentId, workspaceDir, name, filePath }); - return; - } - let safeRead: Awaited>; + let safeRead: Awaited>; try { - safeRead = await agentsHandlerDeps.readLocalFileSafely({ filePath: resolvedPath.ioPath }); + safeRead = await agentsHandlerDeps.readFileWithinRoot({ + rootDir: workspaceDir, + relativePath: name, + rejectHardlinks: true, + nonBlockingRead: true, + }); } catch (err) { if (err instanceof SafeOpenError && err.code === "not-found") { respondWorkspaceFileMissing({ respond, agentId, workspaceDir, name, filePath }); return; } - respondWorkspaceFileUnsafe(respond, name); - return; + if (err instanceof SafeOpenError) { + respondWorkspaceFileUnsafe(respond, name); + return; + } + throw err; } respond( true, @@ -917,36 +749,22 @@ export const agentsHandlers: GatewayRequestHandlers = { const { agentId, workspaceDir, name } = resolved; await fs.mkdir(workspaceDir, { recursive: true }); const filePath = path.join(workspaceDir, name); - const resolvedPath = await resolveWorkspaceFilePathOrRespond({ - respond, - workspaceDir, - name, - }); - if (!resolvedPath) { - return; - } const content = params.content; - const relativeWritePath = path.relative(resolvedPath.workspaceReal, resolvedPath.ioPath); - if ( - !relativeWritePath || - relativeWritePath.startsWith("..") || - path.isAbsolute(relativeWritePath) - ) { - respondWorkspaceFileUnsafe(respond, name); - return; - } try { await agentsHandlerDeps.writeFileWithinRoot({ - rootDir: resolvedPath.workspaceReal, - relativePath: relativeWritePath, + rootDir: workspaceDir, + relativePath: name, data: content, encoding: "utf8", }); - } catch { + } catch (err) { + if (!(err instanceof SafeOpenError)) { + throw err; + } respondWorkspaceFileUnsafe(respond, name); return; } - const meta = await statFileSafely(resolvedPath.ioPath); + const meta = await statWorkspaceFileSafely(workspaceDir, name); respond( true, { diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index f0c7fdcec1c..2dea108b0da 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -1,3 +1,4 @@ +import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -8,10 +9,12 @@ import { import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import * as pinnedPathHelperModule from "./fs-pinned-path-helper.js"; import { + __setFsSafeTestHooksForTest, appendFileWithinRoot, copyFileWithinRoot, createRootScopedReadFile, mkdirPathWithinRoot, + resolveOpenedFileRealPathForHandle, SafeOpenError, openFileWithinRoot, readFileWithinRoot, @@ -25,6 +28,8 @@ import { const tempDirs = createTrackedTempDirs(); afterEach(async () => { + __setFsSafeTestHooksForTest(undefined); + vi.unstubAllEnvs(); await tempDirs.cleanup(); }); @@ -149,6 +154,32 @@ describe("fs-safe", () => { }); }); + it.runIf(process.platform !== "win32")( + "resolves opened file real paths from the fd before the current path target", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const outside = await tempDirs.make("openclaw-fs-safe-outside-"); + const originalPath = path.join(root, "inside.txt"); + const movedPath = path.join(root, "inside-moved.txt"); + const outsidePath = path.join(outside, "outside.txt"); + await fs.writeFile(originalPath, "inside"); + await fs.writeFile(outsidePath, "outside"); + + const handle = await fs.open(originalPath, "r"); + try { + await fs.rename(originalPath, movedPath); + await fs.symlink(outsidePath, originalPath); + + const resolved = await resolveOpenedFileRealPathForHandle(handle, originalPath); + + expect(resolved).toBe(movedPath); + await expect(handle.readFile({ encoding: "utf8" })).resolves.toBe("inside"); + } finally { + await handle.close().catch(() => {}); + } + }, + ); + it("blocks traversal outside root", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); const outside = await tempDirs.make("openclaw-fs-safe-outside-"); @@ -221,6 +252,70 @@ describe("fs-safe", () => { ).rejects.toMatchObject({ code: "invalid-path" }); }); + it.runIf(process.platform !== "win32")( + "rejects symlink-target reads when the path target changes after open", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const insideA = path.join(root, "inside-a.txt"); + const insideB = path.join(root, "inside-b.txt"); + const link = path.join(root, "link.txt"); + await fs.writeFile(insideA, "inside-a"); + await fs.writeFile(insideB, "inside-b"); + await fs.symlink(insideA, link); + + __setFsSafeTestHooksForTest({ + afterOpen: async () => { + await fs.rm(link); + await fs.symlink(insideB, link); + }, + }); + + await expect( + readFileWithinRoot({ + rootDir: root, + relativePath: "link.txt", + allowSymlinkTargetWithinRoot: true, + }), + ).rejects.toMatchObject({ code: "invalid-path" }); + }, + ); + + it("closes the opened handle when afterOpen hook throws", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const filePath = path.join(root, "inside.txt"); + await fs.writeFile(filePath, "inside"); + + let openedHandle: FileHandle | undefined; + __setFsSafeTestHooksForTest({ + afterOpen: (_target, handle) => { + openedHandle = handle; + throw new Error("after-open boom"); + }, + }); + + await expect( + openFileWithinRoot({ + rootDir: root, + relativePath: "inside.txt", + }), + ).rejects.toThrow("after-open boom"); + expect(openedHandle).toBeDefined(); + await expect(openedHandle?.readFile({ encoding: "utf8" })).rejects.toMatchObject({ + code: "EBADF", + }); + }); + + it("rejects setting fs-safe test hooks outside test mode", async () => { + vi.stubEnv("NODE_ENV", "production"); + vi.stubEnv("VITEST", undefined); + + expect(() => + __setFsSafeTestHooksForTest({ + afterPreOpenLstat: () => {}, + }), + ).toThrow("__setFsSafeTestHooksForTest is only available in tests"); + }); + it.runIf(process.platform !== "win32")("blocks hardlink aliases under root", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); const hardlinkPath = path.join(root, "link.txt"); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 3d3d3478a15..8bc90a22016 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -53,11 +53,19 @@ export type SafeLocalReadResult = { export type FsSafeTestHooks = { afterPreOpenLstat?: (filePath: string) => Promise | void; beforeOpen?: (filePath: string, flags: number) => Promise | void; + afterOpen?: (filePath: string, handle: FileHandle) => Promise | void; }; let fsSafeTestHooks: FsSafeTestHooks | undefined; +function allowFsSafeTestHooks(): boolean { + return process.env.NODE_ENV === "test" || process.env.VITEST === "true"; +} + export function __setFsSafeTestHooksForTest(hooks?: FsSafeTestHooks): void { + if (hooks && !allowFsSafeTestHooks()) { + throw new Error("__setFsSafeTestHooksForTest is only available in tests"); + } fsSafeTestHooks = hooks; } @@ -129,6 +137,12 @@ async function openVerifiedLocalFile( : OPEN_READ_FLAGS; await fsSafeTestHooks?.beforeOpen?.(filePath, openFlags); handle = await fs.open(filePath, openFlags); + try { + await fsSafeTestHooks?.afterOpen?.(filePath, handle); + } catch (err) { + await handle.close().catch(() => {}); + throw err; + } } catch (err) { if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "file not found"); @@ -144,24 +158,30 @@ async function openVerifiedLocalFile( } try { - const [stat, pathStat] = await Promise.all([ - handle.stat(), - options?.allowSymlinkTargetWithinRoot ? fs.stat(filePath) : fs.lstat(filePath), - ]); - if (!options?.allowSymlinkTargetWithinRoot && pathStat.isSymbolicLink()) { - throw new SafeOpenError("symlink", "symlink not allowed"); - } + const stat = await handle.stat(); if (!stat.isFile()) { throw new SafeOpenError("not-file", "not a file"); } if (options?.rejectHardlinks && stat.nlink > 1) { throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); } - if (!sameFileIdentity(stat, pathStat)) { - throw new SafeOpenError("path-mismatch", "path changed during read"); + + if (options?.allowSymlinkTargetWithinRoot) { + const pathStat = await fs.stat(filePath); + if (!sameFileIdentity(stat, pathStat)) { + throw new SafeOpenError("path-mismatch", "path changed during read"); + } + } else { + const pathStat = await fs.lstat(filePath); + if (pathStat.isSymbolicLink()) { + throw new SafeOpenError("symlink", "symlink not allowed"); + } + if (!sameFileIdentity(stat, pathStat)) { + throw new SafeOpenError("path-mismatch", "path changed during read"); + } } - const realPath = await fs.realpath(filePath); + const realPath = await resolveOpenedFileRealPathForHandle(handle, filePath); const realStat = await fs.stat(realPath); if (options?.rejectHardlinks && realStat.nlink > 1) { throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); @@ -397,14 +417,6 @@ export async function resolveOpenedFileRealPathForHandle( handle: FileHandle, ioPath: string, ): Promise { - try { - return await fs.realpath(ioPath); - } catch (err) { - if (!isNotFoundPathError(err)) { - throw err; - } - } - const fdCandidates = process.platform === "linux" ? [`/proc/self/fd/${handle.fd}`, `/dev/fd/${handle.fd}`] @@ -418,6 +430,14 @@ export async function resolveOpenedFileRealPathForHandle( // try next fd path } } + + try { + return await fs.realpath(ioPath); + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + } throw new SafeOpenError("path-mismatch", "unable to resolve opened file path"); } @@ -792,6 +812,7 @@ async function resolvePinnedWriteTargetWithinRoot(params: { rootDir: params.rootDir, relativePath: params.relativePath, rejectHardlinks: true, + nonBlockingRead: true, }); try { mode = opened.stat.mode & 0o777;