From 2c211d171e88a8e6adee366d08404f7124764901 Mon Sep 17 00:00:00 2001 From: sudie-codes Date: Sat, 11 Apr 2026 21:33:07 -0700 Subject: [PATCH] fix(msteams): channel file attachments broken by overly-broad HTML fallback (#58617, #51749) (#64645) * fix(msteams): gate channel attachment fallback on tags (#58617, #51749) * test(msteams): remove dead mock branch in graph.test.ts --- .../msteams/src/attachments/graph.test.ts | 153 +++++++++++++++++- extensions/msteams/src/attachments/graph.ts | 109 +++++++------ extensions/msteams/src/attachments/types.ts | 10 ++ .../src/monitor-handler/inbound-media.test.ts | 88 ++++++++-- .../src/monitor-handler/inbound-media.ts | 56 ++++--- 5 files changed, 322 insertions(+), 94 deletions(-) diff --git a/extensions/msteams/src/attachments/graph.test.ts b/extensions/msteams/src/attachments/graph.test.ts index b11486b6e01..021cb9c586c 100644 --- a/extensions/msteams/src/attachments/graph.test.ts +++ b/extensions/msteams/src/attachments/graph.test.ts @@ -96,9 +96,6 @@ function mockGraphMediaFetch(options: { return guardedFetchResult(params, response); } } - if (url.endsWith("/attachments")) { - return guardedFetchResult(params, mockFetchResponse({ value: [] })); - } return guardedFetchResult(params, mockFetchResponse({}, 404)); }); } @@ -267,3 +264,153 @@ describe("downloadMSTeamsGraphMedia hosted content $value fallback", () => { expect(headers.get("User-Agent")).toMatch(/^teams\.ts\[apps\]\/.+ OpenClaw\/.+$/); }); }); + +describe("downloadMSTeamsGraphMedia attachment sourcing and error logging", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("does NOT call the nonexistent ${messageUrl}/attachments sub-resource", async () => { + // The Graph v1.0 API does not expose a `/attachments` sub-resource on + // channel or chat messages. Issue #58617 documented that the old code + // path called this endpoint and recorded a 404 in diagnostics. After + // this fix, the helper must source attachments from the main message + // resource's inline `attachments` array instead. + const fetchCalls: string[] = []; + + mockGraphMediaFetch({ + messageId: "msg-no-sub", + messageResponse: { + body: { content: "hi" }, + attachments: [], + }, + fetchCalls, + }); + + await downloadMSTeamsGraphMedia({ + messageUrl: "https://graph.microsoft.com/v1.0/chats/c/messages/msg-no-sub", + tokenProvider: { getAccessToken: vi.fn(async () => "test-token") }, + maxBytes: 10 * 1024 * 1024, + }); + + const calledSubResource = fetchCalls.some((u) => + u.endsWith("/messages/msg-no-sub/attachments"), + ); + expect(calledSubResource).toBe(false); + }); + + it("sources reference attachments from the message body's attachments array", async () => { + // Before the fix, the helper fetched `/attachments` and used that list. + // After the fix, it must use `msgData.attachments` from the main fetch. + mockGraphMediaFetch({ + messageId: "msg-inline", + messageResponse: { + body: {}, + attachments: [ + { + contentType: "reference", + contentUrl: "https://tenant.sharepoint.com/inline.pdf", + name: "inline.pdf", + }, + ], + }, + }); + vi.mocked(safeFetchWithPolicy).mockResolvedValue(new Response(null, { status: 200 })); + vi.mocked(downloadAndStoreMSTeamsRemoteMedia).mockResolvedValue({ + path: "/tmp/inline.pdf", + contentType: "application/pdf", + placeholder: "[file]", + }); + + const result = await downloadMSTeamsGraphMedia({ + messageUrl: "https://graph.microsoft.com/v1.0/chats/c/messages/msg-inline", + tokenProvider: { getAccessToken: vi.fn(async () => "test-token") }, + maxBytes: 10 * 1024 * 1024, + }); + + expect(result.media).toHaveLength(1); + expect(result.media[0]?.path).toBe("/tmp/inline.pdf"); + // Regression guard: attachmentCount now reflects real inline attachments, + // not the imaginary `/attachments` sub-resource count. + expect(result.attachmentCount).toBe(1); + }); + + it("logs a debug event when the message fetch throws instead of swallowing it", async () => { + // Regression test for #51749: empty `catch {}` blocks used to hide the + // real error, producing misleading `graph media fetch empty` diagnostics + // without surfacing the underlying cause. + vi.mocked(fetchWithSsrFGuard).mockImplementation(async (params: GuardedFetchParams) => { + if (params.url.endsWith("/messages/msg-err")) { + throw new Error("network boom"); + } + // hostedContents and any other paths succeed so the error branch under + // test is the only one that fires. + return guardedFetchResult(params, mockFetchResponse({ value: [] })); + }); + const log = { debug: vi.fn() }; + + const result = await downloadMSTeamsGraphMedia({ + messageUrl: "https://graph.microsoft.com/v1.0/chats/c/messages/msg-err", + tokenProvider: { getAccessToken: vi.fn(async () => "test-token") }, + maxBytes: 10 * 1024 * 1024, + log, + }); + + expect(result.media).toHaveLength(0); + expect(log.debug).toHaveBeenCalledWith( + "graph media message fetch failed", + expect.objectContaining({ error: "network boom" }), + ); + }); + + it("logs a debug event when the message fetch returns non-ok", async () => { + // If the message endpoint returns 403/404, we want that recorded so + // operators can distinguish auth issues from empty result sets. + vi.mocked(fetchWithSsrFGuard).mockImplementation(async (params: GuardedFetchParams) => { + const url = params.url; + if (url.endsWith("/hostedContents")) { + return guardedFetchResult(params, mockFetchResponse({ value: [] })); + } + return guardedFetchResult(params, mockFetchResponse({ error: "forbidden" }, 403)); + }); + const log = { debug: vi.fn() }; + + const result = await downloadMSTeamsGraphMedia({ + messageUrl: "https://graph.microsoft.com/v1.0/chats/c/messages/msg-403", + tokenProvider: { getAccessToken: vi.fn(async () => "test-token") }, + maxBytes: 10 * 1024 * 1024, + log, + }); + + expect(result.media).toHaveLength(0); + expect(result.attachmentStatus).toBe(403); + expect(log.debug).toHaveBeenCalledWith( + "graph media message fetch not ok", + expect.objectContaining({ status: 403 }), + ); + }); + + it("logs a debug event when token acquisition fails", async () => { + vi.mocked(fetchWithSsrFGuard).mockImplementation(async (params: GuardedFetchParams) => + guardedFetchResult(params, mockFetchResponse({})), + ); + const log = { debug: vi.fn() }; + + const result = await downloadMSTeamsGraphMedia({ + messageUrl: "https://graph.microsoft.com/v1.0/chats/c/messages/msg-token", + tokenProvider: { + getAccessToken: vi.fn(async () => { + throw new Error("token expired"); + }), + }, + maxBytes: 10 * 1024 * 1024, + log, + }); + + expect(result.tokenError).toBe(true); + expect(log.debug).toHaveBeenCalledWith( + "graph media token acquisition failed", + expect.objectContaining({ error: "token expired" }), + ); + }); +}); diff --git a/extensions/msteams/src/attachments/graph.ts b/extensions/msteams/src/attachments/graph.ts index ad7b09a7571..fd03dc81a69 100644 --- a/extensions/msteams/src/attachments/graph.ts +++ b/extensions/msteams/src/attachments/graph.ts @@ -27,6 +27,7 @@ import { import type { MSTeamsAccessTokenProvider, MSTeamsAttachmentLike, + MSTeamsGraphMediaLogger, MSTeamsGraphMediaResult, MSTeamsInboundMedia, } from "./types.js"; @@ -281,12 +282,10 @@ export async function downloadMSTeamsGraphMedia(params: { fetchFn?: typeof fetch; /** When true, embeds original filename in stored path for later extraction. */ preserveFilenames?: boolean; - /** - * Optional logger used to surface Graph/SharePoint fetch errors. Without - * it, empty `catch` blocks hide failures like the Node 24+ undici - * incompatibility that silently broke SharePoint downloads (#63396). - */ + /** Optional logger used to surface Graph/SharePoint fetch errors. */ logger?: MSTeamsAttachmentDownloadLogger; + /** Back-compat diagnostic logger used by older tests/callers. */ + log?: MSTeamsGraphMediaLogger; }): Promise { if (!params.messageUrl || !params.tokenProvider) { return { media: [] }; @@ -297,6 +296,8 @@ export async function downloadMSTeamsGraphMedia(params: { }); const ssrfPolicy = resolveMediaSsrfPolicy(policy.allowHosts); const messageUrl = params.messageUrl; + const debugLog = + params.log ?? (params.logger as MSTeamsGraphMediaLogger | undefined) ?? undefined; let accessToken: string; try { accessToken = await params.tokenProvider.getAccessToken("https://graph.microsoft.com"); @@ -307,10 +308,11 @@ export async function downloadMSTeamsGraphMedia(params: { return { media: [], messageUrl, tokenError: true }; } - // Fetch the full message to get SharePoint file attachments (for group chats) const fetchFn = params.fetchFn ?? fetch; const sharePointMedia: MSTeamsInboundMedia[] = []; const downloadedReferenceUrls = new Set(); + let messageAttachments: GraphAttachment[] = []; + let messageStatus: number | undefined; try { const { response: msgRes, release } = await fetchWithSsrFGuard({ url: messageUrl, @@ -322,38 +324,44 @@ export async function downloadMSTeamsGraphMedia(params: { auditContext: "msteams.graph.message", }); try { - if (!msgRes.ok) { - params.logger?.warn?.("msteams graph message fetch non-ok", { - status: msgRes.status, - }); - } + messageStatus = msgRes.status; if (msgRes.ok) { - const msgData = (await msgRes.json()) as { + let msgData: { body?: { content?: string; contentType?: string }; - attachments?: Array<{ - id?: string; - contentUrl?: string; - contentType?: string; - name?: string; - }>; + attachments?: GraphAttachment[]; }; + try { + msgData = (await msgRes.json()) as typeof msgData; + } catch (err) { + debugLog?.debug?.("graph media message parse failed", { + messageUrl, + error: err instanceof Error ? err.message : String(err), + }); + params.logger?.warn?.("msteams graph message parse failed", { + error: err instanceof Error ? err.message : String(err), + messageUrl, + }); + msgData = {}; + } + messageAttachments = Array.isArray(msgData.attachments) ? msgData.attachments : []; - // Extract SharePoint file attachments (contentType: "reference") - // Download any file type, not just images - const spAttachments = (msgData.attachments ?? []).filter( + const spAttachments = messageAttachments.filter( (a) => a.contentType === "reference" && a.contentUrl && a.name, ); for (const att of spAttachments) { const name = att.name ?? "file"; + const shareUrl = att.contentUrl ?? ""; + if (!shareUrl) { + continue; + } try { - // SharePoint URLs need to be accessed via Graph shares API. Validate the - // rewritten Graph URL, not the original SharePoint host, so the existing - // Graph allowlist path can fetch shared files without separately allowing - // arbitrary SharePoint hosts. - const shareUrl = att.contentUrl!; const sharesUrl = `${GRAPH_ROOT}/shares/${encodeGraphShareId(shareUrl)}/driveItem/content`; if (!isUrlAllowed(sharesUrl, policy.allowHosts)) { + debugLog?.debug?.("graph media sharepoint url not in allowHosts", { + messageUrl, + sharesUrl, + }); continue; } @@ -364,11 +372,6 @@ export async function downloadMSTeamsGraphMedia(params: { contentTypeHint: "application/octet-stream", preserveFilenames: params.preserveFilenames, ssrfPolicy, - // The `fetchImpl` below already validates each redirect hop via - // `safeFetchWithPolicy`, so bypass `fetchRemoteMedia`'s strict - // SSRF dispatcher. That dispatcher is incompatible with Node - // 24+'s built-in undici v7 and silently breaks SharePoint - // downloads (#63396). useDirectFetch: true, fetchImpl: async (input, init) => { const requestUrl = resolveRequestUrl(input); @@ -399,6 +402,11 @@ export async function downloadMSTeamsGraphMedia(params: { }); } } + } else { + debugLog?.debug?.("graph media message fetch not ok", { + messageUrl, + status: messageStatus, + }); } } finally { await release(); @@ -419,14 +427,7 @@ export async function downloadMSTeamsGraphMedia(params: { logger: params.logger, }); - const attachments = await fetchGraphCollection({ - url: `${messageUrl}/attachments`, - accessToken, - fetchFn: params.fetchFn, - ssrfPolicy, - }); - - const normalizedAttachments = attachments.items.map(normalizeGraphAttachment); + const normalizedAttachments = messageAttachments.map(normalizeGraphAttachment); const filteredAttachments = sharePointMedia.length > 0 ? normalizedAttachments.filter((att) => { @@ -441,23 +442,31 @@ export async function downloadMSTeamsGraphMedia(params: { return !downloadedReferenceUrls.has(url); }) : normalizedAttachments; - const attachmentMedia = await downloadMSTeamsAttachments({ - attachments: filteredAttachments, - maxBytes: params.maxBytes, - tokenProvider: params.tokenProvider, - allowHosts: policy.allowHosts, - authAllowHosts: policy.authAllowHosts, - fetchFn: params.fetchFn, - preserveFilenames: params.preserveFilenames, - logger: params.logger, - }); + let attachmentMedia: MSTeamsInboundMedia[] = []; + try { + attachmentMedia = await downloadMSTeamsAttachments({ + attachments: filteredAttachments, + maxBytes: params.maxBytes, + tokenProvider: params.tokenProvider, + allowHosts: policy.allowHosts, + authAllowHosts: policy.authAllowHosts, + fetchFn: params.fetchFn, + preserveFilenames: params.preserveFilenames, + logger: params.logger, + }); + } catch (err) { + params.logger?.warn?.("msteams graph attachment download failed", { + error: err instanceof Error ? err.message : String(err), + messageUrl, + }); + } return { media: [...sharePointMedia, ...hosted.media, ...attachmentMedia], hostedCount: hosted.count, attachmentCount: filteredAttachments.length + sharePointMedia.length, hostedStatus: hosted.status, - attachmentStatus: attachments.status, + attachmentStatus: messageStatus, messageUrl, }; } diff --git a/extensions/msteams/src/attachments/types.ts b/extensions/msteams/src/attachments/types.ts index 6750c822c3b..ccf00ac1a18 100644 --- a/extensions/msteams/src/attachments/types.ts +++ b/extensions/msteams/src/attachments/types.ts @@ -35,3 +35,13 @@ export type MSTeamsGraphMediaResult = { messageUrl?: string; tokenError?: boolean; }; + +/** + * Narrow logger surface used by `downloadMSTeamsGraphMedia` for diagnostic + * events. Accepting an optional callback keeps the helper testable without + * pulling in the full channel logger type, while still allowing the monitor + * handler to forward its plugin logger. + */ +export type MSTeamsGraphMediaLogger = { + debug?: (message: string, meta?: Record) => void; +}; diff --git a/extensions/msteams/src/monitor-handler/inbound-media.test.ts b/extensions/msteams/src/monitor-handler/inbound-media.test.ts index 25bfe07e5a2..8e8a2239597 100644 --- a/extensions/msteams/src/monitor-handler/inbound-media.test.ts +++ b/extensions/msteams/src/monitor-handler/inbound-media.test.ts @@ -35,8 +35,9 @@ const baseParams = { }; describe("resolveMSTeamsInboundMedia graph fallback trigger", () => { - it("triggers Graph fallback when some attachments are text/html (some() behavior)", async () => { + it("triggers Graph fallback when HTML contains tags", async () => { vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]); + vi.mocked(extractMSTeamsHtmlAttachmentIds).mockReturnValueOnce(["att-0"]); vi.mocked(downloadMSTeamsGraphMedia).mockResolvedValue({ media: [{ path: "/tmp/img.png", contentType: "image/png", placeholder: "[image]" }], }); @@ -44,8 +45,10 @@ describe("resolveMSTeamsInboundMedia graph fallback trigger", () => { await resolveMSTeamsInboundMedia({ ...baseParams, attachments: [ - { contentType: "text/html", content: "
" }, - { contentType: "image/png", contentUrl: "https://example.com/img.png" }, + { + contentType: "text/html", + content: '
A file
', + }, ], }); @@ -53,8 +56,32 @@ describe("resolveMSTeamsInboundMedia graph fallback trigger", () => { expect(downloadMSTeamsGraphMedia).toHaveBeenCalled(); }); + it("does NOT trigger Graph fallback for mention-only HTML (no tags)", async () => { + vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]); + // Mention cards include `` markers but no ``, + // so the extractor returns an empty ID list. The fallback must skip. + vi.mocked(extractMSTeamsHtmlAttachmentIds).mockReturnValueOnce([]); + vi.mocked(downloadMSTeamsGraphMedia).mockClear(); + vi.mocked(buildMSTeamsGraphMessageUrls).mockClear(); + + await resolveMSTeamsInboundMedia({ + ...baseParams, + attachments: [ + { + contentType: "text/html", + content: '
Bot hello there
', + }, + ], + }); + + expect(downloadMSTeamsGraphMedia).not.toHaveBeenCalled(); + expect(buildMSTeamsGraphMessageUrls).not.toHaveBeenCalled(); + }); + it("does NOT trigger Graph fallback when no attachments are text/html", async () => { vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]); + // No HTML attachments at all → extractor returns []. + vi.mocked(extractMSTeamsHtmlAttachmentIds).mockReturnValueOnce([]); vi.mocked(downloadMSTeamsGraphMedia).mockClear(); vi.mocked(buildMSTeamsGraphMessageUrls).mockClear(); @@ -77,11 +104,44 @@ describe("resolveMSTeamsInboundMedia graph fallback trigger", () => { await resolveMSTeamsInboundMedia({ ...baseParams, - attachments: [{ contentType: "text/html", content: "
" }], + attachments: [ + { + contentType: "text/html", + content: '
', + }, + ], }); expect(downloadMSTeamsGraphMedia).not.toHaveBeenCalled(); }); + + it("forwards log through to downloadMSTeamsGraphMedia for diagnostics", async () => { + vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]); + vi.mocked(extractMSTeamsHtmlAttachmentIds).mockReturnValueOnce(["att-0"]); + vi.mocked(downloadMSTeamsGraphMedia).mockClear(); + vi.mocked(downloadMSTeamsGraphMedia).mockResolvedValue({ media: [] }); + const log = { debug: vi.fn() }; + + await resolveMSTeamsInboundMedia({ + ...baseParams, + log, + attachments: [ + { + contentType: "text/html", + content: '
', + }, + ], + }); + + const call = vi.mocked(downloadMSTeamsGraphMedia).mock.calls[0]?.[0]; + // The monitor handler's logger is forwarded so graph.ts can report + // message fetch failures instead of swallowing them (#51749). + expect(call?.log).toBe(log); + expect(log.debug).toHaveBeenCalledWith( + "graph media fetch empty", + expect.objectContaining({ attachmentIdCount: 1 }), + ); + }); }); describe("resolveMSTeamsInboundMedia bot framework DM routing", () => { @@ -172,23 +232,27 @@ describe("resolveMSTeamsInboundMedia bot framework DM routing", () => { expect(downloadMSTeamsGraphMedia).toHaveBeenCalled(); }); - it("logs when no attachment IDs are present on a BF DM with HTML content", async () => { + it("skips BF DM attachment fetch entirely when HTML has no tags", async () => { vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]); vi.mocked(downloadMSTeamsBotFrameworkAttachments).mockClear(); + vi.mocked(downloadMSTeamsGraphMedia).mockClear(); + // Mention-only HTML (no `` tag) → extractor + // returns []. The fallback skips both the Bot Framework and Graph + // paths so we do not emit spurious 404 diagnostics (#58617). vi.mocked(extractMSTeamsHtmlAttachmentIds).mockReturnValueOnce([]); - const log = { debug: vi.fn() }; await resolveMSTeamsInboundMedia({ ...dmParams, - log, - attachments: [{ contentType: "text/html", content: "
no attachments here
" }], + attachments: [ + { + contentType: "text/html", + content: '
Bot hello
', + }, + ], }); expect(downloadMSTeamsBotFrameworkAttachments).not.toHaveBeenCalled(); - expect(log.debug).toHaveBeenCalledWith( - "bot framework attachment ids unavailable", - expect.objectContaining({ conversationType: "personal" }), - ); + expect(downloadMSTeamsGraphMedia).not.toHaveBeenCalled(); }); it("logs when serviceUrl is missing for a BF DM with HTML content", async () => { diff --git a/extensions/msteams/src/monitor-handler/inbound-media.ts b/extensions/msteams/src/monitor-handler/inbound-media.ts index 7522cbbb38b..a6ccdf53f17 100644 --- a/extensions/msteams/src/monitor-handler/inbound-media.ts +++ b/extensions/msteams/src/monitor-handler/inbound-media.ts @@ -60,52 +60,47 @@ export async function resolveMSTeamsInboundMedia(params: { }); if (mediaList.length === 0) { - const hasHtmlAttachment = attachments.some( - (att) => typeof att.contentType === "string" && att.contentType.startsWith("text/html"), - ); + // Gate the Graph/Bot Framework media fallback on the presence of real + // `` tags inside any `text/html` attachment. Teams + // delivers @mention cards and other chrome as `text/html` attachments + // too, so keying off contentType alone produces spurious 404 diagnostics + // for every mention-only message and masks real file attachments (#58617). + const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments); + const hasHtmlFileAttachment = attachmentIds.length > 0; // Personal DMs with the bot use Bot Framework conversation IDs (`a:...` // or `8:orgid:...`) which Graph's `/chats/{id}` endpoint rejects with // "Invalid ThreadId". Fetch media via the Bot Framework v3 attachments // endpoint instead, which speaks the same identifier space. - if (hasHtmlAttachment && isBotFrameworkPersonalChatId(conversationId)) { + if (hasHtmlFileAttachment && isBotFrameworkPersonalChatId(conversationId)) { if (!serviceUrl) { log.debug?.("bot framework attachment skipped (missing serviceUrl)", { conversationType, conversationId, }); } else { - const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments); - if (attachmentIds.length === 0) { - log.debug?.("bot framework attachment ids unavailable", { - conversationType, - conversationId, - }); + const bfMedia = await downloadMSTeamsBotFrameworkAttachments({ + serviceUrl, + attachmentIds, + tokenProvider, + maxBytes, + allowHosts, + authAllowHosts: params.authAllowHosts, + preserveFilenames, + }); + if (bfMedia.media.length > 0) { + mediaList = bfMedia.media; } else { - const bfMedia = await downloadMSTeamsBotFrameworkAttachments({ - serviceUrl, - attachmentIds, - tokenProvider, - maxBytes, - allowHosts, - authAllowHosts: params.authAllowHosts, - preserveFilenames, - logger: log, + log.debug?.("bot framework attachments fetch empty", { + conversationType, + attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length, }); - if (bfMedia.media.length > 0) { - mediaList = bfMedia.media; - } else { - log.debug?.("bot framework attachments fetch empty", { - conversationType, - attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length, - }); - } } } } if ( - hasHtmlAttachment && + hasHtmlFileAttachment && mediaList.length === 0 && !isBotFrameworkPersonalChatId(conversationId) ) { @@ -160,7 +155,10 @@ export async function resolveMSTeamsInboundMedia(params: { } } if (mediaList.length === 0) { - log.debug?.("graph media fetch empty", { attempts }); + log.debug?.("graph media fetch empty", { + attempts, + attachmentIdCount: attachmentIds.length, + }); } } }