mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-19 11:58:38 +00:00
fix: harden Feishu media URL fetching (#16285) (thanks @mbelinky)
Security fix for Feishu extension media fetching.
This commit is contained in:
committed by
GitHub
parent
d82c5ea9d1
commit
5b4121d601
123
extensions/feishu/src/docx.test.ts
Normal file
123
extensions/feishu/src/docx.test.ts
Normal file
@@ -0,0 +1,123 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const createFeishuClientMock = vi.hoisted(() => vi.fn());
|
||||
const fetchRemoteMediaMock = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock("./client.js", () => ({
|
||||
createFeishuClient: createFeishuClientMock,
|
||||
}));
|
||||
|
||||
vi.mock("./runtime.js", () => ({
|
||||
getFeishuRuntime: () => ({
|
||||
channel: {
|
||||
media: {
|
||||
fetchRemoteMedia: fetchRemoteMediaMock,
|
||||
},
|
||||
},
|
||||
}),
|
||||
}));
|
||||
|
||||
import { registerFeishuDocTools } from "./docx.js";
|
||||
|
||||
describe("feishu_doc image fetch hardening", () => {
|
||||
const convertMock = vi.hoisted(() => vi.fn());
|
||||
const blockListMock = vi.hoisted(() => vi.fn());
|
||||
const blockChildrenCreateMock = vi.hoisted(() => vi.fn());
|
||||
const driveUploadAllMock = vi.hoisted(() => vi.fn());
|
||||
const blockPatchMock = vi.hoisted(() => vi.fn());
|
||||
const scopeListMock = vi.hoisted(() => vi.fn());
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
createFeishuClientMock.mockReturnValue({
|
||||
docx: {
|
||||
document: {
|
||||
convert: convertMock,
|
||||
},
|
||||
documentBlock: {
|
||||
list: blockListMock,
|
||||
patch: blockPatchMock,
|
||||
},
|
||||
documentBlockChildren: {
|
||||
create: blockChildrenCreateMock,
|
||||
},
|
||||
},
|
||||
drive: {
|
||||
media: {
|
||||
uploadAll: driveUploadAllMock,
|
||||
},
|
||||
},
|
||||
application: {
|
||||
scope: {
|
||||
list: scopeListMock,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
convertMock.mockResolvedValue({
|
||||
code: 0,
|
||||
data: {
|
||||
blocks: [{ block_type: 27 }],
|
||||
first_level_block_ids: [],
|
||||
},
|
||||
});
|
||||
|
||||
blockListMock.mockResolvedValue({
|
||||
code: 0,
|
||||
data: {
|
||||
items: [],
|
||||
},
|
||||
});
|
||||
|
||||
blockChildrenCreateMock.mockResolvedValue({
|
||||
code: 0,
|
||||
data: {
|
||||
children: [{ block_type: 27, block_id: "img_block_1" }],
|
||||
},
|
||||
});
|
||||
|
||||
driveUploadAllMock.mockResolvedValue({ file_token: "token_1" });
|
||||
blockPatchMock.mockResolvedValue({ code: 0 });
|
||||
scopeListMock.mockResolvedValue({ code: 0, data: { scopes: [] } });
|
||||
});
|
||||
|
||||
it("skips image upload when markdown image URL is blocked", async () => {
|
||||
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
fetchRemoteMediaMock.mockRejectedValueOnce(
|
||||
new Error("Blocked: resolves to private/internal IP address"),
|
||||
);
|
||||
|
||||
const registerTool = vi.fn();
|
||||
registerFeishuDocTools({
|
||||
config: {
|
||||
channels: {
|
||||
feishu: {
|
||||
appId: "app_id",
|
||||
appSecret: "app_secret",
|
||||
},
|
||||
},
|
||||
} as any,
|
||||
logger: { debug: vi.fn(), info: vi.fn() } as any,
|
||||
registerTool,
|
||||
} as any);
|
||||
|
||||
const feishuDocTool = registerTool.mock.calls
|
||||
.map((call) => call[0])
|
||||
.find((tool) => tool.name === "feishu_doc");
|
||||
expect(feishuDocTool).toBeDefined();
|
||||
|
||||
const result = await feishuDocTool.execute("tool-call", {
|
||||
action: "write",
|
||||
doc_token: "doc_1",
|
||||
content: "",
|
||||
});
|
||||
|
||||
expect(fetchRemoteMediaMock).toHaveBeenCalled();
|
||||
expect(driveUploadAllMock).not.toHaveBeenCalled();
|
||||
expect(blockPatchMock).not.toHaveBeenCalled();
|
||||
expect(result.details.images_processed).toBe(0);
|
||||
expect(consoleErrorSpy).toHaveBeenCalled();
|
||||
consoleErrorSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
@@ -5,6 +5,7 @@ import { Readable } from "stream";
|
||||
import { listEnabledFeishuAccounts } from "./accounts.js";
|
||||
import { createFeishuClient } from "./client.js";
|
||||
import { FeishuDocSchema, type FeishuDocParams } from "./doc-schema.js";
|
||||
import { getFeishuRuntime } from "./runtime.js";
|
||||
import { resolveToolsConfig } from "./tools-config.js";
|
||||
|
||||
// ============ Helpers ============
|
||||
@@ -175,12 +176,9 @@ async function uploadImageToDocx(
|
||||
return fileToken;
|
||||
}
|
||||
|
||||
async function downloadImage(url: string): Promise<Buffer> {
|
||||
const response = await fetch(url);
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to download image: ${response.status} ${response.statusText}`);
|
||||
}
|
||||
return Buffer.from(await response.arrayBuffer());
|
||||
async function downloadImage(url: string, maxBytes: number): Promise<Buffer> {
|
||||
const fetched = await getFeishuRuntime().channel.media.fetchRemoteMedia({ url, maxBytes });
|
||||
return fetched.buffer;
|
||||
}
|
||||
|
||||
/* eslint-disable @typescript-eslint/no-explicit-any -- SDK block types */
|
||||
@@ -189,6 +187,7 @@ async function processImages(
|
||||
docToken: string,
|
||||
markdown: string,
|
||||
insertedBlocks: any[],
|
||||
maxBytes: number,
|
||||
): Promise<number> {
|
||||
/* eslint-enable @typescript-eslint/no-explicit-any */
|
||||
const imageUrls = extractImageUrls(markdown);
|
||||
@@ -204,7 +203,7 @@ async function processImages(
|
||||
const blockId = imageBlocks[i].block_id;
|
||||
|
||||
try {
|
||||
const buffer = await downloadImage(url);
|
||||
const buffer = await downloadImage(url, maxBytes);
|
||||
const urlPath = new URL(url).pathname;
|
||||
const fileName = urlPath.split("/").pop() || `image_${i}.png`;
|
||||
const fileToken = await uploadImageToDocx(client, blockId, buffer, fileName);
|
||||
@@ -284,7 +283,7 @@ async function createDoc(client: Lark.Client, title: string, folderToken?: strin
|
||||
};
|
||||
}
|
||||
|
||||
async function writeDoc(client: Lark.Client, docToken: string, markdown: string) {
|
||||
async function writeDoc(client: Lark.Client, docToken: string, markdown: string, maxBytes: number) {
|
||||
const deleted = await clearDocumentContent(client, docToken);
|
||||
|
||||
const { blocks, firstLevelBlockIds } = await convertMarkdown(client, markdown);
|
||||
@@ -294,7 +293,7 @@ async function writeDoc(client: Lark.Client, docToken: string, markdown: string)
|
||||
const sortedBlocks = sortBlocksByFirstLevel(blocks, firstLevelBlockIds);
|
||||
|
||||
const { children: inserted, skipped } = await insertBlocks(client, docToken, sortedBlocks);
|
||||
const imagesProcessed = await processImages(client, docToken, markdown, inserted);
|
||||
const imagesProcessed = await processImages(client, docToken, markdown, inserted, maxBytes);
|
||||
|
||||
return {
|
||||
success: true,
|
||||
@@ -307,7 +306,12 @@ async function writeDoc(client: Lark.Client, docToken: string, markdown: string)
|
||||
};
|
||||
}
|
||||
|
||||
async function appendDoc(client: Lark.Client, docToken: string, markdown: string) {
|
||||
async function appendDoc(
|
||||
client: Lark.Client,
|
||||
docToken: string,
|
||||
markdown: string,
|
||||
maxBytes: number,
|
||||
) {
|
||||
const { blocks, firstLevelBlockIds } = await convertMarkdown(client, markdown);
|
||||
if (blocks.length === 0) {
|
||||
throw new Error("Content is empty");
|
||||
@@ -315,7 +319,7 @@ async function appendDoc(client: Lark.Client, docToken: string, markdown: string
|
||||
const sortedBlocks = sortBlocksByFirstLevel(blocks, firstLevelBlockIds);
|
||||
|
||||
const { children: inserted, skipped } = await insertBlocks(client, docToken, sortedBlocks);
|
||||
const imagesProcessed = await processImages(client, docToken, markdown, inserted);
|
||||
const imagesProcessed = await processImages(client, docToken, markdown, inserted, maxBytes);
|
||||
|
||||
return {
|
||||
success: true,
|
||||
@@ -453,6 +457,7 @@ export function registerFeishuDocTools(api: OpenClawPluginApi) {
|
||||
// Use first account's config for tools configuration
|
||||
const firstAccount = accounts[0];
|
||||
const toolsCfg = resolveToolsConfig(firstAccount.config.tools);
|
||||
const mediaMaxBytes = (firstAccount.config?.mediaMaxMb ?? 30) * 1024 * 1024;
|
||||
|
||||
// Helper to get client for the default account
|
||||
const getClient = () => createFeishuClient(firstAccount);
|
||||
@@ -475,9 +480,9 @@ export function registerFeishuDocTools(api: OpenClawPluginApi) {
|
||||
case "read":
|
||||
return json(await readDoc(client, p.doc_token));
|
||||
case "write":
|
||||
return json(await writeDoc(client, p.doc_token, p.content));
|
||||
return json(await writeDoc(client, p.doc_token, p.content, mediaMaxBytes));
|
||||
case "append":
|
||||
return json(await appendDoc(client, p.doc_token, p.content));
|
||||
return json(await appendDoc(client, p.doc_token, p.content, mediaMaxBytes));
|
||||
case "create":
|
||||
return json(await createDoc(client, p.title, p.folder_token));
|
||||
case "list_blocks":
|
||||
|
||||
@@ -4,6 +4,7 @@ const createFeishuClientMock = vi.hoisted(() => vi.fn());
|
||||
const resolveFeishuAccountMock = vi.hoisted(() => vi.fn());
|
||||
const normalizeFeishuTargetMock = vi.hoisted(() => vi.fn());
|
||||
const resolveReceiveIdTypeMock = vi.hoisted(() => vi.fn());
|
||||
const loadWebMediaMock = vi.hoisted(() => vi.fn());
|
||||
|
||||
const fileCreateMock = vi.hoisted(() => vi.fn());
|
||||
const messageCreateMock = vi.hoisted(() => vi.fn());
|
||||
@@ -22,6 +23,14 @@ vi.mock("./targets.js", () => ({
|
||||
resolveReceiveIdType: resolveReceiveIdTypeMock,
|
||||
}));
|
||||
|
||||
vi.mock("./runtime.js", () => ({
|
||||
getFeishuRuntime: () => ({
|
||||
media: {
|
||||
loadWebMedia: loadWebMediaMock,
|
||||
},
|
||||
}),
|
||||
}));
|
||||
|
||||
import { sendMediaFeishu } from "./media.js";
|
||||
|
||||
describe("sendMediaFeishu msg_type routing", () => {
|
||||
@@ -31,6 +40,7 @@ describe("sendMediaFeishu msg_type routing", () => {
|
||||
resolveFeishuAccountMock.mockReturnValue({
|
||||
configured: true,
|
||||
accountId: "main",
|
||||
config: {},
|
||||
appId: "app_id",
|
||||
appSecret: "app_secret",
|
||||
domain: "feishu",
|
||||
@@ -65,6 +75,13 @@ describe("sendMediaFeishu msg_type routing", () => {
|
||||
code: 0,
|
||||
data: { message_id: "reply_1" },
|
||||
});
|
||||
|
||||
loadWebMediaMock.mockResolvedValue({
|
||||
buffer: Buffer.from("remote-audio"),
|
||||
fileName: "remote.opus",
|
||||
kind: "audio",
|
||||
contentType: "audio/ogg",
|
||||
});
|
||||
});
|
||||
|
||||
it("uses msg_type=media for mp4", async () => {
|
||||
@@ -148,4 +165,23 @@ describe("sendMediaFeishu msg_type routing", () => {
|
||||
|
||||
expect(messageCreateMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("fails closed when media URL fetch is blocked", async () => {
|
||||
loadWebMediaMock.mockRejectedValueOnce(
|
||||
new Error("Blocked: resolves to private/internal IP address"),
|
||||
);
|
||||
|
||||
await expect(
|
||||
sendMediaFeishu({
|
||||
cfg: {} as any,
|
||||
to: "user:ou_target",
|
||||
mediaUrl: "https://x/img",
|
||||
fileName: "voice.opus",
|
||||
}),
|
||||
).rejects.toThrow(/private\/internal/i);
|
||||
|
||||
expect(fileCreateMock).not.toHaveBeenCalled();
|
||||
expect(messageCreateMock).not.toHaveBeenCalled();
|
||||
expect(messageReplyMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -5,6 +5,7 @@ import path from "path";
|
||||
import { Readable } from "stream";
|
||||
import { resolveFeishuAccount } from "./accounts.js";
|
||||
import { createFeishuClient } from "./client.js";
|
||||
import { getFeishuRuntime } from "./runtime.js";
|
||||
import { resolveReceiveIdType, normalizeFeishuTarget } from "./targets.js";
|
||||
|
||||
export type DownloadImageResult = {
|
||||
@@ -449,23 +450,6 @@ export function detectFileType(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a string is a local file path (not a URL)
|
||||
*/
|
||||
function isLocalPath(urlOrPath: string): boolean {
|
||||
// Starts with / or ~ or drive letter (Windows)
|
||||
if (urlOrPath.startsWith("/") || urlOrPath.startsWith("~") || /^[a-zA-Z]:/.test(urlOrPath)) {
|
||||
return true;
|
||||
}
|
||||
// Try to parse as URL - if it fails or has no protocol, it's likely a local path
|
||||
try {
|
||||
const url = new URL(urlOrPath);
|
||||
return url.protocol === "file:";
|
||||
} catch {
|
||||
return true; // Not a valid URL, treat as local path
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Upload and send media (image or file) from URL, local path, or buffer
|
||||
*/
|
||||
@@ -479,6 +463,11 @@ export async function sendMediaFeishu(params: {
|
||||
accountId?: string;
|
||||
}): Promise<SendMediaResult> {
|
||||
const { cfg, to, mediaUrl, mediaBuffer, fileName, replyToMessageId, accountId } = params;
|
||||
const account = resolveFeishuAccount({ cfg, accountId });
|
||||
if (!account.configured) {
|
||||
throw new Error(`Feishu account "${account.accountId}" not configured`);
|
||||
}
|
||||
const mediaMaxBytes = (account.config?.mediaMaxMb ?? 30) * 1024 * 1024;
|
||||
|
||||
let buffer: Buffer;
|
||||
let name: string;
|
||||
@@ -487,26 +476,12 @@ export async function sendMediaFeishu(params: {
|
||||
buffer = mediaBuffer;
|
||||
name = fileName ?? "file";
|
||||
} else if (mediaUrl) {
|
||||
if (isLocalPath(mediaUrl)) {
|
||||
// Local file path - read directly
|
||||
const filePath = mediaUrl.startsWith("~")
|
||||
? mediaUrl.replace("~", process.env.HOME ?? "")
|
||||
: mediaUrl.replace("file://", "");
|
||||
|
||||
if (!fs.existsSync(filePath)) {
|
||||
throw new Error(`Local file not found: ${filePath}`);
|
||||
}
|
||||
buffer = fs.readFileSync(filePath);
|
||||
name = fileName ?? path.basename(filePath);
|
||||
} else {
|
||||
// Remote URL - fetch
|
||||
const response = await fetch(mediaUrl);
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to fetch media from URL: ${response.status}`);
|
||||
}
|
||||
buffer = Buffer.from(await response.arrayBuffer());
|
||||
name = fileName ?? (path.basename(new URL(mediaUrl).pathname) || "file");
|
||||
}
|
||||
const loaded = await getFeishuRuntime().media.loadWebMedia(mediaUrl, {
|
||||
maxBytes: mediaMaxBytes,
|
||||
optimizeImages: false,
|
||||
});
|
||||
buffer = loaded.buffer;
|
||||
name = fileName ?? loaded.fileName ?? "file";
|
||||
} else {
|
||||
throw new Error("Either mediaUrl or mediaBuffer must be provided");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user