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

@@ -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<ReturnType<typeof ttsMocks.maybeApplyTtsToPayload>>;
 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-"));

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<AcpTurnAttachment[]> {
-  const mediaAttachments = normalizeAttachments(ctx);
+async function resolveAcpAttachments(
+  ctx: FinalizedMsgContext,
+  cfg: OpenClawConfig,
+): Promise<AcpTurnAttachment[]> {
+  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);

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> | 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<LocalReadResult> {
+    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(() => {});
+    }
+  }
 }

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,
+      );
+    });
+  });
 });