fix(msteams): channel file attachments broken by overly-broad HTML fallback (#58617, #51749) (#64645)

* fix(msteams): gate channel attachment fallback on <attachment> tags (#58617, #51749)

* test(msteams): remove dead mock branch in graph.test.ts
This commit is contained in:
sudie-codes
2026-04-11 21:33:07 -07:00
committed by GitHub
parent 95e7af3213
commit 2c211d171e
5 changed files with 322 additions and 94 deletions

View File

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

View File

@@ -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<MSTeamsGraphMediaResult> {
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<string>();
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<GraphAttachment>({
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,
};
}

View File

@@ -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<string, unknown>) => void;
};

View File

@@ -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 <attachment> 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: "<div><img src='x'/></div>" },
{ contentType: "image/png", contentUrl: "https://example.com/img.png" },
{
contentType: "text/html",
content: '<div>A file <attachment id="att-0"></attachment></div>',
},
],
});
@@ -53,8 +56,32 @@ describe("resolveMSTeamsInboundMedia graph fallback trigger", () => {
expect(downloadMSTeamsGraphMedia).toHaveBeenCalled();
});
it("does NOT trigger Graph fallback for mention-only HTML (no <attachment> tags)", async () => {
vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]);
// Mention cards include `<at>` markers but no `<attachment id="...">`,
// 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: '<div><at id="0">Bot</at> hello there</div>',
},
],
});
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: "<div><img src='x'/></div>" }],
attachments: [
{
contentType: "text/html",
content: '<div><attachment id="att-0"></attachment></div>',
},
],
});
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: '<div><attachment id="att-0"></attachment></div>',
},
],
});
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 <attachment> tags", async () => {
vi.mocked(downloadMSTeamsAttachments).mockResolvedValue([]);
vi.mocked(downloadMSTeamsBotFrameworkAttachments).mockClear();
vi.mocked(downloadMSTeamsGraphMedia).mockClear();
// Mention-only HTML (no `<attachment id="...">` 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: "<div>no attachments here</div>" }],
attachments: [
{
contentType: "text/html",
content: '<div><at id="0">Bot</at> hello</div>',
},
],
});
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 () => {

View File

@@ -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
// `<attachment id="...">` 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,
});
}
}
}