From 566fb73d9da2d73c0be0d9b8e5b762e4dcd8e81d Mon Sep 17 00:00:00 2001 From: Jacob Tomlinson Date: Mon, 30 Mar 2026 06:04:02 -0700 Subject: [PATCH] reply: enforce ACP attachment roots (#57690) * reply: enforce ACP attachment roots * media: harden local attachment cache reads * reply: clarify ACP attachment skip logs * reply: keep ACP attachments path-only --- src/auto-reply/reply/dispatch-acp.test.ts | 109 ++++++++++++++++++ src/auto-reply/reply/dispatch-acp.ts | 52 +++++---- src/media-understanding/attachments.cache.ts | 55 ++++++++- .../media-understanding-misc.test.ts | 56 +++++++++ 4 files changed, 248 insertions(+), 24 deletions(-) diff --git a/src/auto-reply/reply/dispatch-acp.test.ts b/src/auto-reply/reply/dispatch-acp.test.ts index e12cbbf27da..0c04bf46f67 100644 --- a/src/auto-reply/reply/dispatch-acp.test.ts +++ b/src/auto-reply/reply/dispatch-acp.test.ts @@ -6,6 +6,7 @@ import { AcpRuntimeError } from "../../acp/runtime/errors.js"; import type { AcpSessionStoreEntry } from "../../acp/runtime/session-meta.js"; import type { OpenClawConfig } from "../../config/config.js"; import type { SessionBindingRecord } from "../../infra/outbound/session-binding-service.js"; +import { withFetchPreconnect } from "../../test-utils/fetch-mock.js"; import type { ReplyDispatcher } from "./reply-dispatcher.js"; import { buildTestCtx } from "./test-ctx.js"; import { createAcpSessionMeta, createAcpTestConfig } from "./test-fixtures/acp-runtime.js"; @@ -58,6 +59,7 @@ const bindingServiceMocks = vi.hoisted(() => ({ })); const sessionKey = "agent:codex-acp:session-1"; +const originalFetch = globalThis.fetch; type MockTtsReply = Awaited>; let tryDispatchAcpReply: typeof import("./dispatch-acp.js").tryDispatchAcpReply; @@ -281,6 +283,7 @@ describe("tryDispatchAcpReply", () => { bindingServiceMocks.listBySession.mockReturnValue([]); bindingServiceMocks.unbind.mockReset(); bindingServiceMocks.unbind.mockResolvedValue([]); + globalThis.fetch = originalFetch; }); it("routes ACP block output to originating channel", async () => { @@ -412,6 +415,13 @@ describe("tryDispatchAcpReply", () => { await runDispatch({ bodyForAgent: " ", + cfg: createAcpTestConfig({ + channels: { + imessage: { + attachmentRoots: [tempDir], + }, + }, + }), ctxOverrides: { MediaPath: imagePath, MediaType: "image/png", @@ -434,6 +444,105 @@ describe("tryDispatchAcpReply", () => { } }); + it("skips ACP attachments outside allowed inbound roots", async () => { + setReadyAcpResolution(); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "dispatch-acp-")); + const imagePath = path.join(tempDir, "outside-root.png"); + try { + await fs.writeFile(imagePath, "image-bytes"); + managerMocks.runTurn.mockResolvedValue(undefined); + + await runDispatch({ + bodyForAgent: " ", + ctxOverrides: { + MediaPath: imagePath, + MediaType: "image/png", + }, + }); + + expect(managerMocks.runTurn).not.toHaveBeenCalled(); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + + it("skips file URL ACP attachments outside allowed inbound roots", async () => { + setReadyAcpResolution(); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "dispatch-acp-")); + const imagePath = path.join(tempDir, "outside-root.png"); + try { + await fs.writeFile(imagePath, "image-bytes"); + managerMocks.runTurn.mockResolvedValue(undefined); + + await runDispatch({ + bodyForAgent: " ", + ctxOverrides: { + MediaPath: `file://${imagePath}`, + MediaType: "image/png", + }, + }); + + expect(managerMocks.runTurn).not.toHaveBeenCalled(); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + + it("skips relative ACP attachment paths that resolve outside allowed inbound roots", async () => { + setReadyAcpResolution(); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "dispatch-acp-")); + const imagePath = path.join(tempDir, "outside-root.png"); + try { + await fs.writeFile(imagePath, "image-bytes"); + managerMocks.runTurn.mockResolvedValue(undefined); + + await runDispatch({ + bodyForAgent: " ", + ctxOverrides: { + MediaPath: path.relative(process.cwd(), imagePath), + MediaType: "image/png", + }, + }); + + expect(managerMocks.runTurn).not.toHaveBeenCalled(); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + + it("does not fall back to remote URLs when ACP local attachment paths are blocked", async () => { + setReadyAcpResolution(); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "dispatch-acp-")); + const imagePath = path.join(tempDir, "outside-root.png"); + const fetchSpy = vi.fn( + async () => + new Response(Buffer.from("remote-image"), { + headers: { + "content-type": "image/png", + }, + }), + ); + globalThis.fetch = withFetchPreconnect(fetchSpy as typeof fetch); + try { + await fs.writeFile(imagePath, "image-bytes"); + managerMocks.runTurn.mockResolvedValue(undefined); + + await runDispatch({ + bodyForAgent: " ", + ctxOverrides: { + MediaPath: imagePath, + MediaUrl: "https://example.com/image.png", + MediaType: "image/png", + }, + }); + + expect(fetchSpy).not.toHaveBeenCalled(); + expect(managerMocks.runTurn).not.toHaveBeenCalled(); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + it("skips ACP turns for non-image attachments when there is no text prompt", async () => { setReadyAcpResolution(); const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "dispatch-acp-")); diff --git a/src/auto-reply/reply/dispatch-acp.ts b/src/auto-reply/reply/dispatch-acp.ts index 2be7d1913a9..2073e76a4c2 100644 --- a/src/auto-reply/reply/dispatch-acp.ts +++ b/src/auto-reply/reply/dispatch-acp.ts @@ -1,4 +1,3 @@ -import fs from "node:fs/promises"; import { getAcpSessionManager } from "../../acp/control-plane/manager.js"; import type { AcpTurnAttachment } from "../../acp/control-plane/manager.types.js"; import { resolveAcpAgentPolicyError, resolveAcpDispatchPolicyError } from "../../acp/policy.js"; @@ -18,10 +17,10 @@ import { getSessionBindingService } from "../../infra/outbound/session-binding-s import { generateSecureUuid } from "../../infra/secure-random.js"; import { prefixSystemMessage } from "../../infra/system-message.js"; import { applyMediaUnderstanding } from "../../media-understanding/apply.js"; -import { - normalizeAttachmentPath, - normalizeAttachments, -} from "../../media-understanding/attachments.normalize.js"; +import { MediaAttachmentCache } from "../../media-understanding/attachments.js"; +import { normalizeAttachments } from "../../media-understanding/attachments.normalize.js"; +import { isMediaUnderstandingSkipError } from "../../media-understanding/errors.js"; +import { resolveMediaAttachmentLocalRoots } from "../../media-understanding/runner.js"; import { resolveAgentIdFromSessionKey } from "../../routing/session-key.js"; import { maybeApplyTtsToPayload, resolveTtsConfig } from "../../tts/tts.js"; import { @@ -69,33 +68,46 @@ function resolveAcpPromptText(ctx: FinalizedMsgContext): string { } const ACP_ATTACHMENT_MAX_BYTES = 10 * 1024 * 1024; +const ACP_ATTACHMENT_TIMEOUT_MS = 1_000; -async function resolveAcpAttachments(ctx: FinalizedMsgContext): Promise { - const mediaAttachments = normalizeAttachments(ctx); +async function resolveAcpAttachments( + ctx: FinalizedMsgContext, + cfg: OpenClawConfig, +): Promise { + const mediaAttachments = normalizeAttachments(ctx).map((attachment) => + attachment.path?.trim() ? { ...attachment, url: undefined } : attachment, + ); + const cache = new MediaAttachmentCache(mediaAttachments, { + localPathRoots: resolveMediaAttachmentLocalRoots({ cfg, ctx }), + }); const results: AcpTurnAttachment[] = []; for (const attachment of mediaAttachments) { const mediaType = attachment.mime ?? "application/octet-stream"; if (!mediaType.startsWith("image/")) { continue; } - const filePath = normalizeAttachmentPath(attachment.path); - if (!filePath) { + if (!attachment.path?.trim()) { continue; } try { - const stat = await fs.stat(filePath); - if (stat.size > ACP_ATTACHMENT_MAX_BYTES) { - logVerbose( - `dispatch-acp: skipping attachment ${filePath} (${stat.size} bytes exceeds ${ACP_ATTACHMENT_MAX_BYTES} byte limit)`, - ); - continue; - } - const buf = await fs.readFile(filePath); + const { buffer } = await cache.getBuffer({ + attachmentIndex: attachment.index, + maxBytes: ACP_ATTACHMENT_MAX_BYTES, + timeoutMs: ACP_ATTACHMENT_TIMEOUT_MS, + }); results.push({ mediaType, - data: buf.toString("base64"), + data: buffer.toString("base64"), }); - } catch { + } catch (error) { + if (isMediaUnderstandingSkipError(error)) { + logVerbose(`dispatch-acp: skipping attachment #${attachment.index + 1} (${error.reason})`); + } else { + const errorName = error instanceof Error ? error.name : typeof error; + logVerbose( + `dispatch-acp: failed to read attachment #${attachment.index + 1} (${errorName})`, + ); + } // Skip unreadable files. Text content should still be delivered. } } @@ -429,7 +441,7 @@ export async function tryDispatchAcpReply(params: { } const promptText = resolveAcpPromptText(params.ctx); - const attachments = await resolveAcpAttachments(params.ctx); + const attachments = await resolveAcpAttachments(params.ctx, params.cfg); if (!promptText && attachments.length === 0) { const counts = params.dispatcher.getQueuedCounts(); delivery.applyRoutedCounts(counts); diff --git a/src/media-understanding/attachments.cache.ts b/src/media-understanding/attachments.cache.ts index fce32e3dc6d..ab3761a9932 100644 --- a/src/media-understanding/attachments.cache.ts +++ b/src/media-understanding/attachments.cache.ts @@ -1,3 +1,4 @@ +import { constants as fsConstants } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { logVerbose, shouldLogVerbose } from "../globals.js"; @@ -28,6 +29,11 @@ type MediaPathResult = { cleanup?: () => Promise | void; }; +type LocalReadResult = { + buffer: Buffer; + filePath: string; +}; + type AttachmentCacheEntry = { attachment: MediaAttachment; resolvedPath?: string; @@ -110,17 +116,21 @@ export class MediaAttachmentCache { `Attachment ${params.attachmentIndex + 1} exceeds maxBytes ${params.maxBytes}`, ); } - const buffer = await fs.readFile(entry.resolvedPath); + const { buffer, filePath } = await this.readLocalBuffer({ + attachmentIndex: params.attachmentIndex, + filePath: entry.resolvedPath, + maxBytes: params.maxBytes, + }); + entry.resolvedPath = filePath; entry.buffer = buffer; entry.bufferMime = entry.bufferMime ?? entry.attachment.mime ?? (await detectMime({ buffer, - filePath: entry.resolvedPath, + filePath, })); - entry.bufferFileName = - path.basename(entry.resolvedPath) || `media-${params.attachmentIndex + 1}`; + entry.bufferFileName = path.basename(filePath) || `media-${params.attachmentIndex + 1}`; return { buffer, mime: entry.bufferMime, @@ -328,4 +338,41 @@ export class MediaAttachmentCache { ))(); return await this.canonicalLocalPathRoots; } + + private async readLocalBuffer(params: { + attachmentIndex: number; + filePath: string; + maxBytes: number; + }): Promise { + const flags = + fsConstants.O_RDONLY | (process.platform === "win32" ? 0 : fsConstants.O_NOFOLLOW); + const handle = await fs.open(params.filePath, flags); + try { + const stat = await handle.stat(); + if (!stat.isFile()) { + throw new MediaUnderstandingSkipError( + "empty", + `Attachment ${params.attachmentIndex + 1} has no path or URL.`, + ); + } + const canonicalPath = await fs.realpath(params.filePath).catch(() => params.filePath); + const canonicalRoots = await this.getCanonicalLocalPathRoots(); + if (!isInboundPathAllowed({ filePath: canonicalPath, roots: canonicalRoots })) { + throw new MediaUnderstandingSkipError( + "empty", + `Attachment ${params.attachmentIndex + 1} has no path or URL.`, + ); + } + const buffer = await handle.readFile(); + if (buffer.length > params.maxBytes) { + throw new MediaUnderstandingSkipError( + "maxBytes", + `Attachment ${params.attachmentIndex + 1} exceeds maxBytes ${params.maxBytes}`, + ); + } + return { buffer, filePath: canonicalPath }; + } finally { + await handle.close().catch(() => {}); + } + } } diff --git a/src/media-understanding/media-understanding-misc.test.ts b/src/media-understanding/media-understanding-misc.test.ts index 9279ce5e670..4b02099b546 100644 --- a/src/media-understanding/media-understanding-misc.test.ts +++ b/src/media-understanding/media-understanding-misc.test.ts @@ -1,3 +1,4 @@ +import { constants as fsConstants } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -117,4 +118,59 @@ describe("media understanding attachments SSRF", () => { ).rejects.toThrow(/has no path or URL/i); }); }); + + it("enforces maxBytes after reading local attachments", async () => { + await withTempRoot("openclaw-media-cache-max-bytes-", async (base) => { + const allowedRoot = path.join(base, "allowed"); + const attachmentPath = path.join(allowedRoot, "voice-note.m4a"); + await fs.mkdir(allowedRoot, { recursive: true }); + await fs.writeFile(attachmentPath, "ok"); + + const cache = new MediaAttachmentCache([{ index: 0, path: attachmentPath }], { + localPathRoots: [allowedRoot], + }); + const originalOpen = fs.open.bind(fs); + const openSpy = vi.spyOn(fs, "open"); + + openSpy.mockImplementation(async (filePath, flags) => { + const handle = await originalOpen(filePath, flags); + if (filePath !== attachmentPath) { + return handle; + } + const mockedHandle = handle as typeof handle & { + readFile: typeof handle.readFile; + }; + mockedHandle.readFile = (async () => Buffer.alloc(2048, 1)) as typeof handle.readFile; + return mockedHandle; + }); + + await expect( + cache.getBuffer({ attachmentIndex: 0, maxBytes: 1024, timeoutMs: 1000 }), + ).rejects.toThrow(/exceeds maxBytes 1024/i); + }); + }); + + it("opens local attachments with nofollow on posix", async () => { + if (process.platform === "win32") { + return; + } + await withTempRoot("openclaw-media-cache-flags-", async (base) => { + const allowedRoot = path.join(base, "allowed"); + const attachmentPath = path.join(allowedRoot, "voice-note.m4a"); + await fs.mkdir(allowedRoot, { recursive: true }); + await fs.writeFile(attachmentPath, "ok"); + + const cache = new MediaAttachmentCache([{ index: 0, path: attachmentPath }], { + localPathRoots: [allowedRoot], + }); + const openSpy = vi.spyOn(fs, "open"); + + await cache.getBuffer({ attachmentIndex: 0, maxBytes: 1024, timeoutMs: 1000 }); + + expect(openSpy).toHaveBeenCalledWith( + attachmentPath, + fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW, + ); + }); + }); });