From 1fed7bc3796e37c303297cd925b7688ba63fba2b Mon Sep 17 00:00:00 2001 From: sudie-codes Date: Wed, 8 Apr 2026 19:22:12 -0700 Subject: [PATCH] fix(msteams): pin reply target at inbound time to prevent DM/channel leak (#54520) (#62716) --- .../src/conversation-store-helpers.test.ts | 202 ++++++++++++++++++ .../msteams/src/conversation-store-helpers.ts | 49 +++-- extensions/msteams/src/send-context.ts | 15 ++ 3 files changed, 249 insertions(+), 17 deletions(-) create mode 100644 extensions/msteams/src/conversation-store-helpers.test.ts diff --git a/extensions/msteams/src/conversation-store-helpers.test.ts b/extensions/msteams/src/conversation-store-helpers.test.ts new file mode 100644 index 00000000000..d4342103953 --- /dev/null +++ b/extensions/msteams/src/conversation-store-helpers.test.ts @@ -0,0 +1,202 @@ +import { describe, expect, it } from "vitest"; +import { findPreferredDmConversationByUserId } from "./conversation-store-helpers.js"; +import type { MSTeamsConversationStoreEntry } from "./conversation-store.js"; + +function entry(params: { + conversationId: string; + userId?: string; + aadObjectId?: string; + conversationType?: string; + lastSeenAt?: string; +}): MSTeamsConversationStoreEntry { + return { + conversationId: params.conversationId, + reference: { + user: { + id: params.userId ?? "user-1", + aadObjectId: params.aadObjectId ?? "aad-1", + }, + conversation: { + id: params.conversationId, + conversationType: params.conversationType, + }, + lastSeenAt: params.lastSeenAt, + }, + }; +} + +describe("findPreferredDmConversationByUserId", () => { + it("returns null for empty id", () => { + expect(findPreferredDmConversationByUserId([], " ")).toBeNull(); + }); + + it("returns null when no entries match", () => { + const entries = [entry({ conversationId: "conv-1", aadObjectId: "other-user" })]; + expect(findPreferredDmConversationByUserId(entries, "aad-1")).toBeNull(); + }); + + it("returns a personal DM conversation by aadObjectId", () => { + const entries = [ + entry({ + conversationId: "dm-conv", + aadObjectId: "aad-target", + conversationType: "personal", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("dm-conv"); + }); + + it("returns a personal DM conversation by user.id", () => { + const entries = [ + entry({ + conversationId: "dm-conv", + userId: "user-target", + aadObjectId: "other", + conversationType: "personal", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "user-target"); + expect(result?.conversationId).toBe("dm-conv"); + }); + + it("does NOT return a channel conversation for a user lookup (#54520)", () => { + // This is the core bug: user sends messages in both a DM and a channel. + // The channel conversation also carries the user's aadObjectId. + // findPreferredDmByUserId must NOT return the channel conversation. + const entries = [ + entry({ + conversationId: "19:channel@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "channel", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result).toBeNull(); + }); + + it("does NOT return a groupChat conversation for a user lookup (#54520)", () => { + const entries = [ + entry({ + conversationId: "19:group@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "groupChat", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result).toBeNull(); + }); + + it("prefers personal DM over channel even when channel is more recent (#54520)", () => { + // Reproduces the exact race: channel message arrives after DM, but the + // DM conversation should still be returned. + const entries = [ + entry({ + conversationId: "dm-conv", + aadObjectId: "aad-target", + conversationType: "personal", + lastSeenAt: "2026-03-25T20:00:00.000Z", + }), + entry({ + conversationId: "19:channel@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "channel", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("dm-conv"); + }); + + it("prefers personal DM over groupChat even when groupChat is more recent", () => { + const entries = [ + entry({ + conversationId: "dm-conv", + aadObjectId: "aad-target", + conversationType: "personal", + lastSeenAt: "2026-03-25T20:00:00.000Z", + }), + entry({ + conversationId: "19:group@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "groupChat", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("dm-conv"); + }); + + it("prefers the freshest personal DM when multiple exist", () => { + const entries = [ + entry({ + conversationId: "dm-old", + aadObjectId: "aad-target", + conversationType: "personal", + lastSeenAt: "2026-03-25T20:00:00.000Z", + }), + entry({ + conversationId: "dm-new", + aadObjectId: "aad-target", + conversationType: "personal", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("dm-new"); + }); + + it("falls back to unknown-type entries when no personal conversations exist", () => { + // Legacy entries without conversationType should still be usable as a + // fallback to avoid breaking existing deployments. + const entries = [ + entry({ + conversationId: "legacy-conv", + aadObjectId: "aad-target", + // No conversationType set (legacy entry) + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("legacy-conv"); + }); + + it("prefers personal over unknown-type entries", () => { + const entries = [ + entry({ + conversationId: "legacy-conv", + aadObjectId: "aad-target", + lastSeenAt: "2026-03-25T21:00:00.000Z", + // No conversationType + }), + entry({ + conversationId: "dm-conv", + aadObjectId: "aad-target", + conversationType: "personal", + lastSeenAt: "2026-03-25T20:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result?.conversationId).toBe("dm-conv"); + }); + + it("does NOT fall back to channel/group when no personal or unknown entries exist", () => { + const entries = [ + entry({ + conversationId: "19:channel@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "channel", + lastSeenAt: "2026-03-25T21:00:00.000Z", + }), + entry({ + conversationId: "19:group@thread.tacv2", + aadObjectId: "aad-target", + conversationType: "groupChat", + lastSeenAt: "2026-03-25T20:00:00.000Z", + }), + ]; + const result = findPreferredDmConversationByUserId(entries, "aad-target"); + expect(result).toBeNull(); + }); +}); diff --git a/extensions/msteams/src/conversation-store-helpers.ts b/extensions/msteams/src/conversation-store-helpers.ts index b1879b853e8..3737f7c059f 100644 --- a/extensions/msteams/src/conversation-store-helpers.ts +++ b/extensions/msteams/src/conversation-store-helpers.ts @@ -51,30 +51,45 @@ export function findPreferredDmConversationByUserId( return null; } - const matches: MSTeamsConversationStoreEntry[] = []; + // Partition user matches into DM-safe and non-DM buckets. + // Channel and group conversations also carry the sender's aadObjectId, but + // returning one of those when the caller asked for a user-targeted DM would + // leak the reply into a shared channel -- the root cause of #54520. + const personalMatches: MSTeamsConversationStoreEntry[] = []; + const unknownTypeMatches: MSTeamsConversationStoreEntry[] = []; for (const entry of entries) { - if (entry.reference.user?.aadObjectId === target || entry.reference.user?.id === target) { - matches.push(entry); + if (entry.reference.user?.aadObjectId !== target && entry.reference.user?.id !== target) { + continue; + } + const convType = normalizeLowercaseStringOrEmpty( + entry.reference.conversation?.conversationType ?? "", + ); + if (convType === "personal") { + personalMatches.push(entry); + } else if (convType === "channel" || convType === "groupchat") { + // Explicitly skip channel/group conversations -- these must never be + // returned for a user-targeted DM lookup. + } else { + // Legacy entries without conversationType are ambiguous. Include them + // as a fallback but rank below confirmed personal conversations. + unknownTypeMatches.push(entry); } } - if (matches.length === 0) { + // Prefer confirmed personal DMs, fall back to unknown-type entries. + const candidates = personalMatches.length > 0 ? personalMatches : unknownTypeMatches; + if (candidates.length === 0) { return null; } - matches.sort((a, b) => { - const aType = normalizeLowercaseStringOrEmpty(a.reference.conversation?.conversationType ?? ""); - const bType = normalizeLowercaseStringOrEmpty(b.reference.conversation?.conversationType ?? ""); - const aPersonal = aType === "personal" ? 1 : 0; - const bPersonal = bType === "personal" ? 1 : 0; - if (aPersonal !== bPersonal) { - return bPersonal - aPersonal; - } - return ( - (parseStoredConversationTimestamp(b.reference.lastSeenAt) ?? 0) - - (parseStoredConversationTimestamp(a.reference.lastSeenAt) ?? 0) + // When multiple candidates exist, prefer the most recently seen one. + if (candidates.length > 1) { + candidates.sort( + (a, b) => + (parseStoredConversationTimestamp(b.reference.lastSeenAt) ?? 0) - + (parseStoredConversationTimestamp(a.reference.lastSeenAt) ?? 0), ); - }); + } - return matches[0] ?? null; + return candidates[0] ?? null; } diff --git a/extensions/msteams/src/send-context.ts b/extensions/msteams/src/send-context.ts index 790708231e9..80c883ff9ae 100644 --- a/extensions/msteams/src/send-context.ts +++ b/extensions/msteams/src/send-context.ts @@ -130,6 +130,21 @@ export async function resolveMSTeamsSendContext(params: { } const { conversationId, ref } = found; + + // Safety check: when the caller targeted a specific user (DM), verify the + // resolved conversation is actually a personal DM. Without this guard a + // stale or mismatched conversation store could route a private DM reply + // into a shared channel or group chat -- see #54520. + if (recipient.type === "user") { + const resolvedType = normalizeLowercaseStringOrEmpty(ref.conversation?.conversationType ?? ""); + if (resolvedType && resolvedType !== "personal") { + throw new Error( + `Conversation reference for user:${recipient.id} resolved to a ${resolvedType} ` + + `conversation (${conversationId}) instead of a personal DM. ` + + `The bot must receive a DM from this user before it can send proactively.`, + ); + } + } const core = getMSTeamsRuntime(); const log = core.logging.getChildLogger({ name: "msteams:send" });