diff --git a/extensions/feishu/src/bot.ts b/extensions/feishu/src/bot.ts index a14a6b8baf..14b787d36d 100644 --- a/extensions/feishu/src/bot.ts +++ b/extensions/feishu/src/bot.ts @@ -7,11 +7,14 @@ import { DEFAULT_GROUP_HISTORY_LIMIT, type HistoryEntry, } from "openclaw/plugin-sdk"; +import type { FeishuMessageContext, FeishuMediaInfo, ResolvedFeishuAccount } from "./types.js"; +import type { DynamicAgentCreationConfig } from "./types.js"; import { resolveFeishuAccount } from "./accounts.js"; import { createFeishuClient } from "./client.js"; import { tryRecordMessage } from "./dedup.js"; import { maybeCreateDynamicAgent } from "./dynamic-agent.js"; -import { downloadImageFeishu, downloadMessageResourceFeishu } from "./media.js"; +import { normalizeFeishuExternalKey } from "./external-keys.js"; +import { downloadMessageResourceFeishu } from "./media.js"; import { extractMentionTargets, extractMessageBody, isMentionForwardRequest } from "./mention.js"; import { resolveFeishuGroupConfig, @@ -22,8 +25,6 @@ import { import { createFeishuReplyDispatcher } from "./reply-dispatcher.js"; import { getFeishuRuntime } from "./runtime.js"; import { getMessageFeishu, sendMessageFeishu } from "./send.js"; -import type { FeishuMessageContext, FeishuMediaInfo, ResolvedFeishuAccount } from "./types.js"; -import type { DynamicAgentCreationConfig } from "./types.js"; // --- Permission error extraction --- // Extract permission grant URL from Feishu API error response. @@ -224,18 +225,20 @@ function parseMediaKeys( } { try { const parsed = JSON.parse(content); + const imageKey = normalizeFeishuExternalKey(parsed.image_key); + const fileKey = normalizeFeishuExternalKey(parsed.file_key); switch (messageType) { case "image": - return { imageKey: parsed.image_key }; + return { imageKey }; case "file": - return { fileKey: parsed.file_key, fileName: parsed.file_name }; + return { fileKey, fileName: parsed.file_name }; case "audio": - return { fileKey: parsed.file_key }; + return { fileKey }; case "video": // Video has both file_key (video) and image_key (thumbnail) - return { fileKey: parsed.file_key, imageKey: parsed.image_key }; + return { fileKey, imageKey }; case "sticker": - return { fileKey: parsed.file_key }; + return { fileKey }; default: return {}; } @@ -277,7 +280,10 @@ function parsePostContent(content: string): { } } else if (element.tag === "img" && element.image_key) { // Embedded image - imageKeys.push(element.image_key); + const imageKey = normalizeFeishuExternalKey(element.image_key); + if (imageKey) { + imageKeys.push(imageKey); + } } } textContent += "\n"; diff --git a/extensions/feishu/src/external-keys.test.ts b/extensions/feishu/src/external-keys.test.ts new file mode 100644 index 0000000000..69acdcb0d7 --- /dev/null +++ b/extensions/feishu/src/external-keys.test.ts @@ -0,0 +1,20 @@ +import { describe, expect, it } from "vitest"; +import { normalizeFeishuExternalKey } from "./external-keys.js"; + +describe("normalizeFeishuExternalKey", () => { + it("accepts a normal feishu key and trims surrounding spaces", () => { + expect(normalizeFeishuExternalKey(" img_v3_01abcDEF123 ")).toBe("img_v3_01abcDEF123"); + }); + + it("rejects traversal and path separator patterns", () => { + expect(normalizeFeishuExternalKey("../etc/passwd")).toBeUndefined(); + expect(normalizeFeishuExternalKey("a/../../b")).toBeUndefined(); + expect(normalizeFeishuExternalKey("a\\..\\b")).toBeUndefined(); + }); + + it("rejects empty, non-string, and control-char values", () => { + expect(normalizeFeishuExternalKey(" ")).toBeUndefined(); + expect(normalizeFeishuExternalKey(123)).toBeUndefined(); + expect(normalizeFeishuExternalKey("abc\u0000def")).toBeUndefined(); + }); +}); diff --git a/extensions/feishu/src/external-keys.ts b/extensions/feishu/src/external-keys.ts new file mode 100644 index 0000000000..c896759a12 --- /dev/null +++ b/extensions/feishu/src/external-keys.ts @@ -0,0 +1,19 @@ +const CONTROL_CHARS_RE = /[\u0000-\u001f\u007f]/; +const MAX_EXTERNAL_KEY_LENGTH = 512; + +export function normalizeFeishuExternalKey(value: unknown): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const normalized = value.trim(); + if (!normalized || normalized.length > MAX_EXTERNAL_KEY_LENGTH) { + return undefined; + } + if (CONTROL_CHARS_RE.test(normalized)) { + return undefined; + } + if (normalized.includes("/") || normalized.includes("\\") || normalized.includes("..")) { + return undefined; + } + return normalized; +} diff --git a/extensions/feishu/src/media.test.ts b/extensions/feishu/src/media.test.ts index 1ea15d8ace..b9e97703a1 100644 --- a/extensions/feishu/src/media.test.ts +++ b/extensions/feishu/src/media.test.ts @@ -199,8 +199,8 @@ describe("sendMediaFeishu msg_type routing", () => { expect(messageReplyMock).not.toHaveBeenCalled(); }); - it("does not include imageKey path segments in temp file path", async () => { - const maliciousImageKey = "a/../../../../pwned.txt"; + it("uses isolated temp paths for image downloads", async () => { + const imageKey = "img_v3_01abc123"; let capturedPath: string | undefined; imageGetMock.mockResolvedValueOnce({ @@ -212,12 +212,12 @@ describe("sendMediaFeishu msg_type routing", () => { const result = await downloadImageFeishu({ cfg: {} as any, - imageKey: maliciousImageKey, + imageKey, }); expect(result.buffer).toEqual(Buffer.from("image-data")); expect(capturedPath).toBeDefined(); - expect(capturedPath).not.toContain(maliciousImageKey); + expect(capturedPath).not.toContain(imageKey); expect(capturedPath).not.toContain(".."); const tmpRoot = path.resolve(os.tmpdir()); @@ -226,8 +226,8 @@ describe("sendMediaFeishu msg_type routing", () => { expect(rel === ".." || rel.startsWith(`..${path.sep}`)).toBe(false); }); - it("does not include fileKey path segments in temp file path", async () => { - const maliciousFileKey = "x/../../../../../etc/hosts"; + it("uses isolated temp paths for message resource downloads", async () => { + const fileKey = "file_v3_01abc123"; let capturedPath: string | undefined; messageResourceGetMock.mockResolvedValueOnce({ @@ -240,13 +240,13 @@ describe("sendMediaFeishu msg_type routing", () => { const result = await downloadMessageResourceFeishu({ cfg: {} as any, messageId: "om_123", - fileKey: maliciousFileKey, + fileKey, type: "image", }); expect(result.buffer).toEqual(Buffer.from("resource-data")); expect(capturedPath).toBeDefined(); - expect(capturedPath).not.toContain(maliciousFileKey); + expect(capturedPath).not.toContain(fileKey); expect(capturedPath).not.toContain(".."); const tmpRoot = path.resolve(os.tmpdir()); @@ -254,4 +254,28 @@ describe("sendMediaFeishu msg_type routing", () => { const rel = path.relative(tmpRoot, resolved); expect(rel === ".." || rel.startsWith(`..${path.sep}`)).toBe(false); }); + + it("rejects invalid image keys before calling feishu api", async () => { + await expect( + downloadImageFeishu({ + cfg: {} as any, + imageKey: "a/../../bad", + }), + ).rejects.toThrow("invalid image_key"); + + expect(imageGetMock).not.toHaveBeenCalled(); + }); + + it("rejects invalid file keys before calling feishu api", async () => { + await expect( + downloadMessageResourceFeishu({ + cfg: {} as any, + messageId: "om_123", + fileKey: "x/../../bad", + type: "file", + }), + ).rejects.toThrow("invalid file_key"); + + expect(messageResourceGetMock).not.toHaveBeenCalled(); + }); }); diff --git a/extensions/feishu/src/media.ts b/extensions/feishu/src/media.ts index 90a4c5d8c5..a614ee191a 100644 --- a/extensions/feishu/src/media.ts +++ b/extensions/feishu/src/media.ts @@ -1,10 +1,10 @@ import fs from "fs"; -import os from "os"; +import { withTempDownloadPath, type ClawdbotConfig } from "openclaw/plugin-sdk"; import path from "path"; import { Readable } from "stream"; -import type { ClawdbotConfig } from "openclaw/plugin-sdk"; import { resolveFeishuAccount } from "./accounts.js"; import { createFeishuClient } from "./client.js"; +import { normalizeFeishuExternalKey } from "./external-keys.js"; import { getFeishuRuntime } from "./runtime.js"; import { assertFeishuMessageApiSuccess, toFeishuSendResult } from "./send-result.js"; import { resolveReceiveIdType, normalizeFeishuTarget } from "./targets.js"; @@ -20,19 +20,6 @@ export type DownloadMessageResourceResult = { fileName?: string; }; -async function withTempDownloadPath( - prefix: string, - fn: (tmpPath: string) => Promise, -): Promise { - const dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), prefix)); - const tmpPath = path.join(dir, "download.bin"); - try { - return await fn(tmpPath); - } finally { - await fs.promises.rm(dir, { recursive: true, force: true }).catch(() => {}); - } -} - async function readFeishuResponseBuffer(params: { response: unknown; tmpDirPrefix: string; @@ -66,7 +53,7 @@ async function readFeishuResponseBuffer(params: { return Buffer.concat(chunks); } if (typeof responseAny.writeFile === "function") { - return await withTempDownloadPath(params.tmpDirPrefix, async (tmpPath) => { + return await withTempDownloadPath({ prefix: params.tmpDirPrefix }, async (tmpPath) => { await responseAny.writeFile(tmpPath); return await fs.promises.readFile(tmpPath); }); @@ -101,6 +88,10 @@ export async function downloadImageFeishu(params: { accountId?: string; }): Promise { const { cfg, imageKey, accountId } = params; + const normalizedImageKey = normalizeFeishuExternalKey(imageKey); + if (!normalizedImageKey) { + throw new Error("Feishu image download failed: invalid image_key"); + } const account = resolveFeishuAccount({ cfg, accountId }); if (!account.configured) { throw new Error(`Feishu account "${account.accountId}" not configured`); @@ -109,7 +100,7 @@ export async function downloadImageFeishu(params: { const client = createFeishuClient(account); const response = await client.im.image.get({ - path: { image_key: imageKey }, + path: { image_key: normalizedImageKey }, }); const buffer = await readFeishuResponseBuffer({ @@ -132,6 +123,10 @@ export async function downloadMessageResourceFeishu(params: { accountId?: string; }): Promise { const { cfg, messageId, fileKey, type, accountId } = params; + const normalizedFileKey = normalizeFeishuExternalKey(fileKey); + if (!normalizedFileKey) { + throw new Error("Feishu message resource download failed: invalid file_key"); + } const account = resolveFeishuAccount({ cfg, accountId }); if (!account.configured) { throw new Error(`Feishu account "${account.accountId}" not configured`); @@ -140,7 +135,7 @@ export async function downloadMessageResourceFeishu(params: { const client = createFeishuClient(account); const response = await client.im.messageResource.get({ - path: { message_id: messageId, file_key: fileKey }, + path: { message_id: messageId, file_key: normalizedFileKey }, params: { type }, }); diff --git a/src/media-understanding/attachments.ts b/src/media-understanding/attachments.ts index 5976886a9a..14952b5319 100644 --- a/src/media-understanding/attachments.ts +++ b/src/media-understanding/attachments.ts @@ -1,17 +1,16 @@ -import crypto from "node:crypto"; import fs from "node:fs/promises"; -import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import type { MsgContext } from "../auto-reply/templating.js"; import type { MediaUnderstandingAttachmentsConfig } from "../config/types.tools.js"; +import type { MediaAttachment, MediaUnderstandingCapability } from "./types.js"; import { logVerbose, shouldLogVerbose } from "../globals.js"; import { isAbortError } from "../infra/unhandled-rejections.js"; import { fetchRemoteMedia, MediaFetchError } from "../media/fetch.js"; import { detectMime, getFileExtension, isAudioFileName, kindFromMime } from "../media/mime.js"; +import { buildRandomTempFilePath } from "../plugin-sdk/temp-path.js"; import { MediaUnderstandingSkipError } from "./errors.js"; import { fetchWithTimeout } from "./providers/shared.js"; -import type { MediaAttachment, MediaUnderstandingCapability } from "./types.js"; type MediaBufferResult = { buffer: Buffer; @@ -352,7 +351,10 @@ export class MediaAttachmentCache { timeoutMs: params.timeoutMs, }); const extension = path.extname(bufferResult.fileName || "") || ""; - const tmpPath = path.join(os.tmpdir(), `openclaw-media-${crypto.randomUUID()}${extension}`); + const tmpPath = buildRandomTempFilePath({ + prefix: "openclaw-media", + extension, + }); await fs.writeFile(tmpPath, bufferResult.buffer); entry.tempPath = tmpPath; entry.tempCleanup = async () => { diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index b1c2a92812..c18892c6fa 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -154,7 +154,7 @@ export { extractToolSend } from "./tool-send.js"; export { resolveChannelAccountConfigBasePath } from "./config-paths.js"; export { chunkTextForOutbound } from "./text-chunking.js"; export { readJsonFileWithFallback, writeJsonFileAtomically } from "./json-store.js"; -export { buildRandomTempFilePath } from "./temp-path.js"; +export { buildRandomTempFilePath, withTempDownloadPath } from "./temp-path.js"; export type { ChatType } from "../channels/chat-type.js"; /** @deprecated Use ChatType instead */ export type { RoutePeerKind } from "../routing/resolve-route.js"; diff --git a/src/plugin-sdk/temp-path.test.ts b/src/plugin-sdk/temp-path.test.ts index 63f307f98b..dbd2d46ee0 100644 --- a/src/plugin-sdk/temp-path.test.ts +++ b/src/plugin-sdk/temp-path.test.ts @@ -1,7 +1,8 @@ +import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { buildRandomTempFilePath } from "./temp-path.js"; +import { buildRandomTempFilePath, withTempDownloadPath } from "./temp-path.js"; describe("buildRandomTempFilePath", () => { it("builds deterministic paths when now/uuid are provided", () => { @@ -30,3 +31,41 @@ describe("buildRandomTempFilePath", () => { expect(result).not.toContain(".."); }); }); + +describe("withTempDownloadPath", () => { + it("creates a temp path under tmp dir and cleans up the temp directory", async () => { + let capturedPath = ""; + await withTempDownloadPath( + { + prefix: "line-media", + }, + async (tmpPath) => { + capturedPath = tmpPath; + await fs.writeFile(tmpPath, "ok"); + }, + ); + + expect(capturedPath).toContain(path.join(os.tmpdir(), "line-media-")); + await expect(fs.stat(capturedPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("sanitizes prefix and fileName", async () => { + let capturedPath = ""; + await withTempDownloadPath( + { + prefix: "../../line/../media", + fileName: "../../evil.bin", + }, + async (tmpPath) => { + capturedPath = tmpPath; + }, + ); + + const tmpRoot = path.resolve(os.tmpdir()); + const resolved = path.resolve(capturedPath); + const rel = path.relative(tmpRoot, resolved); + expect(rel === ".." || rel.startsWith(`..${path.sep}`)).toBe(false); + expect(path.basename(capturedPath)).toBe("evil.bin"); + expect(capturedPath).not.toContain(".."); + }); +}); diff --git a/src/plugin-sdk/temp-path.ts b/src/plugin-sdk/temp-path.ts index f140012884..ed1b149135 100644 --- a/src/plugin-sdk/temp-path.ts +++ b/src/plugin-sdk/temp-path.ts @@ -1,4 +1,5 @@ import crypto from "node:crypto"; +import { mkdtemp, rm } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -20,6 +21,12 @@ function sanitizeExtension(extension?: string): string { return `.${token}`; } +function sanitizeFileName(fileName: string): string { + const base = path.basename(fileName).replace(/[^a-zA-Z0-9._-]+/g, "-"); + const normalized = base.replace(/^-+|-+$/g, ""); + return normalized || "download.bin"; +} + export function buildRandomTempFilePath(params: { prefix: string; extension?: string; @@ -37,3 +44,22 @@ export function buildRandomTempFilePath(params: { const uuid = params.uuid?.trim() || crypto.randomUUID(); return path.join(params.tmpDir ?? os.tmpdir(), `${prefix}-${now}-${uuid}${extension}`); } + +export async function withTempDownloadPath( + params: { + prefix: string; + fileName?: string; + tmpDir?: string; + }, + fn: (tmpPath: string) => Promise, +): Promise { + const tempRoot = params.tmpDir ?? os.tmpdir(); + const prefix = `${sanitizePrefix(params.prefix)}-`; + const dir = await mkdtemp(path.join(tempRoot, prefix)); + const tmpPath = path.join(dir, sanitizeFileName(params.fileName ?? "download.bin")); + try { + return await fn(tmpPath); + } finally { + await rm(dir, { recursive: true, force: true }).catch(() => {}); + } +} diff --git a/src/security/temp-path-guard.test.ts b/src/security/temp-path-guard.test.ts new file mode 100644 index 0000000000..f21172e41c --- /dev/null +++ b/src/security/temp-path-guard.test.ts @@ -0,0 +1,63 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +const DYNAMIC_TMPDIR_JOIN_RE = /path\.join\(os\.tmpdir\(\),\s*`[^`]*\$\{[^`]*`/; +const RUNTIME_ROOTS = ["src", "extensions"]; +const SKIP_PATTERNS = [ + /\.test\.tsx?$/, + /\.e2e\.tsx?$/, + /\.d\.ts$/, + /[\\/](?:__tests__|tests)[\\/]/, + /[\\/]test-helpers(?:\.[^\\/]+)?\.ts$/, +]; + +function shouldSkip(relativePath: string): boolean { + return SKIP_PATTERNS.some((pattern) => pattern.test(relativePath)); +} + +async function listTsFiles(dir: string): Promise { + const entries = await fs.readdir(dir, { withFileTypes: true }); + const out: string[] = []; + for (const entry of entries) { + if (entry.name === "node_modules" || entry.name === "dist" || entry.name.startsWith(".")) { + continue; + } + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + out.push(...(await listTsFiles(fullPath))); + continue; + } + if (!entry.isFile()) { + continue; + } + if (fullPath.endsWith(".ts") || fullPath.endsWith(".tsx")) { + out.push(fullPath); + } + } + return out; +} + +describe("temp path guard", () => { + it("blocks dynamic template path.join(os.tmpdir(), ...) in runtime source files", async () => { + const repoRoot = process.cwd(); + const offenders: string[] = []; + + for (const root of RUNTIME_ROOTS) { + const absRoot = path.join(repoRoot, root); + const files = await listTsFiles(absRoot); + for (const file of files) { + const relativePath = path.relative(repoRoot, file); + if (shouldSkip(relativePath)) { + continue; + } + const source = await fs.readFile(file, "utf-8"); + if (DYNAMIC_TMPDIR_JOIN_RE.test(source)) { + offenders.push(relativePath); + } + } + } + + expect(offenders).toEqual([]); + }); +});