From 7af2bb62622e1999c0dc2ecc7ef7bf4290987894 Mon Sep 17 00:00:00 2001 From: wangmiao0668000666 Date: Fri, 31 Jul 2026 20:09:28 +0800 Subject: [PATCH] fix(file-transfer): keep fetched media attachable in sandboxed replies (#116400) Fixes #116338 Co-authored-by: wangmiao0668000666 --- .../file-transfer/src/tools/descriptors.ts | 6 +- .../src/tools/dir-fetch-tool.test.ts | 19 +++- .../file-transfer/src/tools/dir-fetch-tool.ts | 2 +- .../src/tools/file-fetch-tool.test.ts | 13 +-- .../src/tools/file-write-tool.test.ts | 38 +++++++- src/agents/sandbox-paths.test.ts | 96 ++++++++++++++----- 6 files changed, 134 insertions(+), 40 deletions(-) diff --git a/extensions/file-transfer/src/tools/descriptors.ts b/extensions/file-transfer/src/tools/descriptors.ts index b7bb1ea9ab38..4c0675168252 100644 --- a/extensions/file-transfer/src/tools/descriptors.ts +++ b/extensions/file-transfer/src/tools/descriptors.ts @@ -8,9 +8,9 @@ type FileTransferToolDescriptor = Pick< "label" | "name" | "description" | "parameters" >; -// Stash fetched files in a non-TTL subdir so follow-up tool calls within -// the same turn can still reference them. -export const FILE_TRANSFER_SUBDIR = "file-transfer"; +// Keep fetched files in the managed tool-media namespace so sandboxed replies +// can attach them and follow-up file_write calls can reuse the media id. +export const FILE_TRANSFER_SUBDIR = "tool-file-transfer"; export const FILE_FETCH_DEFAULT_MAX_BYTES = 8 * 1024 * 1024; export const FILE_FETCH_HARD_MAX_BYTES = 16 * 1024 * 1024; diff --git a/extensions/file-transfer/src/tools/dir-fetch-tool.test.ts b/extensions/file-transfer/src/tools/dir-fetch-tool.test.ts index 656d5880122b..8fa3cd1a20dc 100644 --- a/extensions/file-transfer/src/tools/dir-fetch-tool.test.ts +++ b/extensions/file-transfer/src/tools/dir-fetch-tool.test.ts @@ -5,6 +5,7 @@ import os from "node:os"; import path from "node:path"; import * as tar from "tar"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { DIR_FETCH_HARD_MAX_BYTES, FILE_TRANSFER_SUBDIR } from "./descriptors.js"; let tmpRoot: string; @@ -37,12 +38,13 @@ async function createTarBuffer(params: { async function importTool(tarBuffer: Buffer) { const archivePath = path.join(tmpRoot, `archive-${randomUUID()}.tar.gz`); const appendFileTransferAudit = vi.fn(async () => undefined); + const saveMediaBuffer = vi.fn(async () => { + await fs.writeFile(archivePath, tarBuffer); + return { path: archivePath }; + }); vi.resetModules(); vi.doMock("openclaw/plugin-sdk/media-store", () => ({ - saveMediaBuffer: vi.fn(async () => { - await fs.writeFile(archivePath, tarBuffer); - return { path: archivePath }; - }), + saveMediaBuffer, })); vi.doMock("../shared/audit.js", () => ({ appendFileTransferAudit })); vi.doMock("./node-tool-invoke.js", () => ({ @@ -67,6 +69,7 @@ async function importTool(tarBuffer: Buffer) { return { archivePath, appendFileTransferAudit, + saveMediaBuffer, module: await import("./dir-fetch-tool.js"), }; } @@ -86,7 +89,7 @@ describe("dir.fetch archive extraction", () => { await fs.writeFile(path.join(sourceDir, "ok.txt"), "ok"); }, }); - const { appendFileTransferAudit, module } = await importTool(tarBuffer); + const { appendFileTransferAudit, module, saveMediaBuffer } = await importTool(tarBuffer); const result = await executeDirFetch(module); @@ -106,6 +109,12 @@ describe("dir.fetch archive extraction", () => { const localPath = (result.details as { files: Array<{ localPath: string }> }).files[0] ?.localPath; await expect(fs.readFile(localPath!, "utf8")).resolves.toBe("ok"); + expect(saveMediaBuffer).toHaveBeenCalledWith( + tarBuffer, + "application/gzip", + FILE_TRANSFER_SUBDIR, + DIR_FETCH_HARD_MAX_BYTES, + ); expect(appendFileTransferAudit).toHaveBeenLastCalledWith( expect.objectContaining({ decision: "allowed" }), ); diff --git a/extensions/file-transfer/src/tools/dir-fetch-tool.ts b/extensions/file-transfer/src/tools/dir-fetch-tool.ts index 265d6369e4dc..9c196a41d0d5 100644 --- a/extensions/file-transfer/src/tools/dir-fetch-tool.ts +++ b/extensions/file-transfer/src/tools/dir-fetch-tool.ts @@ -169,7 +169,7 @@ export function createDirFetchTool(): AnyAgentTool { throw new Error("dir.fetch sha256 mismatch (integrity failure)"); } - // Save tarball under the file-transfer subdir (no 2-min TTL). + // Keep the tarball and extracted paths under the same managed tool namespace. const savedTar = await saveMediaBuffer( tarBuffer, "application/gzip", diff --git a/extensions/file-transfer/src/tools/file-fetch-tool.test.ts b/extensions/file-transfer/src/tools/file-fetch-tool.test.ts index 221872335c2f..c64bf4154b25 100644 --- a/extensions/file-transfer/src/tools/file-fetch-tool.test.ts +++ b/extensions/file-transfer/src/tools/file-fetch-tool.test.ts @@ -7,6 +7,7 @@ import { } from "openclaw/plugin-sdk/agent-harness-runtime"; import { saveMediaBuffer } from "openclaw/plugin-sdk/media-store"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { FILE_TRANSFER_SUBDIR } from "./descriptors.js"; import { createFileFetchTool } from "./file-fetch-tool.js"; vi.mock("openclaw/plugin-sdk/agent-harness-runtime", () => ({ @@ -57,7 +58,7 @@ describe("file_fetch tool", () => { }); vi.mocked(saveMediaBuffer).mockResolvedValue({ id: "media-1", - path: "/gateway/media/file-transfer/report.md", + path: "/gateway/media/tool-file-transfer/report.md", size: Buffer.byteLength(fileText), contentType: "text/markdown", }); @@ -95,7 +96,7 @@ describe("file_fetch tool", () => { }); vi.mocked(saveMediaBuffer).mockResolvedValue({ id: "media-1", - path: "/gateway/media/file-transfer/bom.md", + path: "/gateway/media/tool-file-transfer/bom.md", size: originalBuffer.byteLength, contentType: "text/markdown", }); @@ -111,7 +112,7 @@ describe("file_fetch tool", () => { expect(saveMediaBuffer).toHaveBeenCalledWith( originalBuffer, "text/markdown", - expect.any(String), + FILE_TRANSFER_SUBDIR, expect.any(Number), ); const details = result.details as { sha256: string; size: number }; @@ -134,7 +135,7 @@ describe("file_fetch tool", () => { }); vi.mocked(saveMediaBuffer).mockResolvedValue({ id: "media-1", - path: "/gateway/media/file-transfer/empty.png", + path: "/gateway/media/tool-file-transfer/empty.png", size: 0, contentType: "image/png", }); @@ -148,7 +149,7 @@ describe("file_fetch tool", () => { expect(result.content[0]?.type).toBe("text"); const text = result.content[0]?.type === "text" ? result.content[0].text : ""; expect(text).toContain("Fetched /tmp/empty.png"); - expect(text).toContain("saved at /gateway/media/file-transfer/empty.png"); + expect(text).toContain("saved at /gateway/media/tool-file-transfer/empty.png"); }); it("still inlines a non-empty image payload", async () => { @@ -167,7 +168,7 @@ describe("file_fetch tool", () => { }); vi.mocked(saveMediaBuffer).mockResolvedValue({ id: "media-1", - path: "/gateway/media/file-transfer/photo.png", + path: "/gateway/media/tool-file-transfer/photo.png", size: buffer.byteLength, contentType: "image/png", }); diff --git a/extensions/file-transfer/src/tools/file-write-tool.test.ts b/extensions/file-transfer/src/tools/file-write-tool.test.ts index add0520f4ee3..d34cb7177343 100644 --- a/extensions/file-transfer/src/tools/file-write-tool.test.ts +++ b/extensions/file-transfer/src/tools/file-write-tool.test.ts @@ -1,12 +1,14 @@ // File Transfer tests cover file write tool plugin behavior. +import crypto from "node:crypto"; import { callGatewayTool, listNodes, resolveNodeIdFromList, } from "openclaw/plugin-sdk/agent-harness-runtime"; +import { readMediaBuffer } from "openclaw/plugin-sdk/media-store"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { humanSize } from "../shared/params.js"; -import { FILE_WRITE_HARD_MAX_BYTES } from "./descriptors.js"; +import { FILE_TRANSFER_SUBDIR, FILE_WRITE_HARD_MAX_BYTES } from "./descriptors.js"; import { createFileWriteTool } from "./file-write-tool.js"; vi.mock("openclaw/plugin-sdk/agent-harness-runtime", () => ({ @@ -101,4 +103,38 @@ describe("file_write tool", () => { expect(callGatewayTool).toHaveBeenCalledOnce(); }); + + it("reads file_fetch media from the shared managed tool namespace", async () => { + const buffer = Buffer.from("copied"); + vi.mocked(readMediaBuffer).mockResolvedValue({ + id: "media-1", + buffer, + path: "/gateway/media/tool-file-transfer/media-1.bin", + size: buffer.byteLength, + }); + vi.mocked(listNodes).mockResolvedValue([{ nodeId: "node-1", displayName: "Node 1" }]); + vi.mocked(resolveNodeIdFromList).mockReturnValue("node-1"); + vi.mocked(callGatewayTool).mockResolvedValue({ + payload: { + ok: true, + path: "/tmp/out.bin", + size: buffer.byteLength, + sha256: crypto.createHash("sha256").update(buffer).digest("hex"), + overwritten: false, + }, + }); + + const result = await createFileWriteTool().execute("tool-call-1", { + node: "node-1", + path: "/tmp/out.bin", + sourceMediaId: "media-1", + }); + + expect(readMediaBuffer).toHaveBeenCalledWith( + "media-1", + FILE_TRANSFER_SUBDIR, + FILE_WRITE_HARD_MAX_BYTES, + ); + expect(result.details).toMatchObject({ source: "media", size: buffer.byteLength }); + }); }); diff --git a/src/agents/sandbox-paths.test.ts b/src/agents/sandbox-paths.test.ts index 102d9bfd8272..79d5f940feeb 100644 --- a/src/agents/sandbox-paths.test.ts +++ b/src/agents/sandbox-paths.test.ts @@ -41,6 +41,7 @@ async function withManagedMediaRoot(run: (ctx: { stateDir: string }) => Promi try { return await withEnvAsync({ OPENCLAW_STATE_DIR: stateDir }, async () => { await fs.mkdir(path.join(stateDir, "media", "outbound"), { recursive: true }); + await fs.mkdir(path.join(stateDir, "media", "tool-file-transfer"), { recursive: true }); await fs.mkdir(path.join(stateDir, "media", "tool-image-generation"), { recursive: true }); return await run({ stateDir }); }); @@ -242,6 +243,10 @@ describe("resolveSandboxedMediaSource", () => { name: "managed outbound media", relative: path.join("media", "outbound", "reply.png"), }, + { + name: "managed file-transfer tool media", + relative: path.join("media", "tool-file-transfer", "fetched.png"), + }, { name: "managed tool media", relative: path.join("media", "tool-image-generation", "generated.png"), @@ -475,47 +480,90 @@ describe("resolveSandboxedMediaSource", () => { ); }); - it("rejects symlinked managed media paths escaping the managed media root", async () => { - if (process.platform === "win32") { - return; - } - await withManagedMediaRoot(async ({ stateDir }) => { - await withSandboxRoot(async (sandboxDir) => { + it.each(["outbound", "tool-file-transfer"])( + "rejects symlinked managed media paths escaping the %s root", + async (subdir) => { + if (process.platform === "win32") { + return; + } + await withManagedMediaRoot(async ({ stateDir }) => { + await withSandboxRoot(async (sandboxDir) => { + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "managed-media-outside-")); + const outsideFile = path.join(outsideDir, "secret.png"); + const symlinkPath = path.join(stateDir, "media", subdir, "linked-secret.png"); + try { + await fs.writeFile(outsideFile, "secret", "utf8"); + await fs.symlink(outsideFile, symlinkPath); + + await expectSandboxRejection(symlinkPath, sandboxDir, /managed media root|symlink/i); + } finally { + await fs.rm(symlinkPath, { force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + }); + }, + ); + + it.each(["outbound", "tool-file-transfer"])( + "rejects checked managed media symlinks escaping the %s root", + async (subdir) => { + if (process.platform === "win32") { + return; + } + await withManagedMediaRoot(async ({ stateDir }) => { const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "managed-media-outside-")); const outsideFile = path.join(outsideDir, "secret.png"); - const symlinkPath = path.join(stateDir, "media", "outbound", "linked-secret.png"); + const symlinkPath = path.join(stateDir, "media", subdir, "linked-secret.png"); try { await fs.writeFile(outsideFile, "secret", "utf8"); await fs.symlink(outsideFile, symlinkPath); - await expectSandboxRejection(symlinkPath, sandboxDir, /managed media root|symlink/i); + await expect(resolveAllowedManagedMediaPath(symlinkPath)).rejects.toThrow( + /managed media root|symlink/i, + ); } finally { await fs.rm(symlinkPath, { force: true }); await fs.rm(outsideDir, { recursive: true, force: true }); } }); - }); - }); + }, + ); - it("rejects checked managed media symlinks escaping the managed media root", async () => { + it("rejects hardlinked file-transfer media that aliases a file outside managed media", async () => { if (process.platform === "win32") { return; } await withManagedMediaRoot(async ({ stateDir }) => { - const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "managed-media-outside-")); - const outsideFile = path.join(outsideDir, "secret.png"); - const symlinkPath = path.join(stateDir, "media", "outbound", "linked-secret.png"); - try { - await fs.writeFile(outsideFile, "secret", "utf8"); - await fs.symlink(outsideFile, symlinkPath); - - await expect(resolveAllowedManagedMediaPath(symlinkPath)).rejects.toThrow( - /managed media root|symlink/i, + await withSandboxRoot(async (sandboxDir) => { + const outsideDir = await fs.mkdtemp( + path.join(path.dirname(stateDir), "managed-media-hardlink-outside-"), ); - } finally { - await fs.rm(symlinkPath, { force: true }); - await fs.rm(outsideDir, { recursive: true, force: true }); - } + const outsideFile = path.join(outsideDir, "secret.png"); + const hardlinkPath = path.join( + stateDir, + "media", + "tool-file-transfer", + "linked-secret.png", + ); + try { + await fs.writeFile(outsideFile, "secret", "utf8"); + try { + await fs.link(outsideFile, hardlinkPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + await expect(resolveAllowedManagedMediaPath(hardlinkPath)).rejects.toThrow(/hard.?link/i); + await expectSandboxRejection(hardlinkPath, sandboxDir, /hard.?link|managed media root/i); + } finally { + await fs.rm(hardlinkPath, { force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); }); });