From 80b1fa17bfc3f6a668492f0326ea52f48bb89776 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Mon, 13 Apr 2026 23:52:30 +0530 Subject: [PATCH] fix(msteams): enforce sender allowlist checks on SSO signin invokes [AI] (#66033) * fix: address issue * fix: address PR review feedback * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + .../msteams/src/monitor-handler.sso.test.ts | 152 +++++++++++++++++- extensions/msteams/src/monitor-handler.ts | 61 ++++++- 3 files changed, 203 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa660242e22..56fcab64a85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(msteams): enforce sender allowlist checks on SSO signin invokes [AI]. (#66033) Thanks @pgondhi987. - fix(config): redact sourceConfig and runtimeConfig alias fields in redactConfigSnapshot [AI]. (#66030) Thanks @pgondhi987. - Agents/context engines: run opt-in turn maintenance as idle-aware background work so the next foreground turn no longer waits on proactive maintenance. (#65233) thanks @100yenadmin diff --git a/extensions/msteams/src/monitor-handler.sso.test.ts b/extensions/msteams/src/monitor-handler.sso.test.ts index 599d60d559d..a2c0139b0c9 100644 --- a/extensions/msteams/src/monitor-handler.sso.test.ts +++ b/extensions/msteams/src/monitor-handler.sso.test.ts @@ -1,5 +1,5 @@ import { beforeAll, describe, expect, it, vi } from "vitest"; -import type { PluginRuntime } from "../runtime-api.js"; +import type { OpenClawConfig, PluginRuntime } from "../runtime-api.js"; import { type MSTeamsActivityHandler, type MSTeamsMessageHandlerDeps, @@ -95,7 +95,20 @@ function createSigninInvokeContext(params: { value: unknown; userAadId?: string; userBfId?: string; + conversationId?: string; + conversationType?: "personal" | "groupChat" | "channel"; + teamId?: string; + channelName?: string; }): MSTeamsTurnContext & { sendActivity: ReturnType } { + const conversationType = params.conversationType ?? "personal"; + const conversationId = + params.conversationId ?? + (conversationType === "personal" + ? "19:personal-chat" + : conversationType === "channel" + ? "19:channel@thread.tacv2" + : "19:group@thread.tacv2"); + return { activity: { id: "invoke-1", @@ -110,10 +123,16 @@ function createSigninInvokeContext(params: { }, recipient: { id: "bot-id", name: "Bot" }, conversation: { - id: "19:personal-chat", - conversationType: "personal", + id: conversationId, + conversationType, + tenantId: params.teamId ? "tenant-1" : undefined, }, - channelData: {}, + channelData: params.teamId + ? { + team: { id: params.teamId, name: "Team 1" }, + channel: params.channelName ? { name: params.channelName } : undefined, + } + : {}, attachments: [], value: params.value, }, @@ -150,6 +169,69 @@ function createFakeFetch(handlers: Array<(url: string, init?: unknown) => unknow return { fetchImpl, calls }; } +function createBlockedSigninScenarios() { + return [ + { + name: "DM sender outside allowlist", + cfg: { + channels: { + msteams: { + dmPolicy: "allowlist", + allowFrom: ["owner-aad"], + }, + }, + } as OpenClawConfig, + context: { + userAadId: "blocked-dm-aad", + }, + expectedDropLog: "dropping signin invoke (dm sender not allowlisted)", + }, + { + name: "channel outside route allowlist", + cfg: { + channels: { + msteams: { + groupPolicy: "allowlist", + groupAllowFrom: ["blocked-channel-aad"], + teams: { + "team-allowlisted": { + channels: { + "19:allowlisted@thread.tacv2": { requireMention: false }, + }, + }, + }, + }, + }, + } as OpenClawConfig, + context: { + userAadId: "blocked-channel-aad", + conversationType: "channel" as const, + conversationId: "19:blocked-channel@thread.tacv2", + teamId: "team-blocked", + channelName: "General", + }, + expectedDropLog: "dropping signin invoke (not in team/channel allowlist)", + }, + { + name: "group sender outside group allowlist", + cfg: { + channels: { + msteams: { + groupPolicy: "allowlist", + groupAllowFrom: ["owner-aad"], + }, + }, + } as OpenClawConfig, + context: { + userAadId: "blocked-group-aad", + conversationType: "groupChat" as const, + conversationId: "19:group-chat@thread.v2", + }, + expectedDropLog: "dropping signin invoke (group sender not allowlisted)", + }, + ]; +} + describe("msteams signin invoke value parsers", () => { it("parses signin/tokenExchange values", () => { expect( @@ -321,6 +403,18 @@ describe("msteams signin invoke handler registration", () => { installTestRuntime(); }); + const blockedSigninScenarios = createBlockedSigninScenarios(); + const invokeVariants = [ + { + name: "signin/tokenExchange" as const, + value: { id: "x", connectionName: "GraphConnection", token: "exchangeable" }, + }, + { + name: "signin/verifyState" as const, + value: { state: "112233" }, + }, + ]; + it("acks signin invokes even when sso is not configured", async () => { const deps = createDepsWithoutSso(); const { handler, run } = createActivityHandler(); @@ -348,6 +442,56 @@ describe("msteams signin invoke handler registration", () => { ); }); + for (const invoke of invokeVariants) { + for (const scenario of blockedSigninScenarios) { + it(`does not process ${invoke.name} for ${scenario.name}`, async () => { + const { fetchImpl, calls } = createFakeFetch([ + () => ({ + ok: true, + status: 200, + body: { + channelId: "msteams", + connectionName: "GraphConnection", + token: "delegated-graph-token", + expiration: "2030-01-01T00:00:00Z", + }, + }), + ]); + const { sso, tokenStore } = createSsoDeps({ fetchImpl }); + const deps = createDepsWithoutSso({ cfg: scenario.cfg, sso }); + const { handler } = createActivityHandler(); + const registered = registerMSTeamsHandlers(handler, deps) as MSTeamsActivityHandler & { + run: NonNullable; + }; + + const ctx = createSigninInvokeContext({ + name: invoke.name, + value: invoke.value, + ...scenario.context, + }); + + await registered.run(ctx); + + expect(ctx.sendActivity).toHaveBeenCalledWith( + expect.objectContaining({ + type: "invokeResponse", + value: expect.objectContaining({ status: 200 }), + }), + ); + expect(calls).toHaveLength(0); + const stored = await tokenStore.get({ + connectionName: "GraphConnection", + userId: scenario.context.userAadId ?? "aad-user-guid", + }); + expect(stored).toBeNull(); + expect(deps.log.debug).toHaveBeenCalledWith( + scenario.expectedDropLog, + expect.objectContaining({ name: invoke.name }), + ); + }); + } + } + it("invokes the token exchange handler when sso is configured", async () => { const { fetchImpl } = createFakeFetch([ () => ({ diff --git a/extensions/msteams/src/monitor-handler.ts b/extensions/msteams/src/monitor-handler.ts index 7e4768e2122..53c75f89a7c 100644 --- a/extensions/msteams/src/monitor-handler.ts +++ b/extensions/msteams/src/monitor-handler.ts @@ -56,10 +56,17 @@ function serializeAdaptiveCardActionValue(value: unknown): string | null { } } -async function isFeedbackInvokeAuthorized( - context: MSTeamsTurnContext, - deps: MSTeamsMessageHandlerDeps, -): Promise { +async function isInvokeAuthorized(params: { + context: MSTeamsTurnContext; + deps: MSTeamsMessageHandlerDeps; + deniedLogs: { + dm: string; + channel: string; + group: string; + }; + includeInvokeName?: boolean; +}): Promise { + const { context, deps, deniedLogs, includeInvokeName = false } = params; const resolved = await resolveMSTeamsSenderAccess({ cfg: deps.cfg, activity: context.activity, @@ -69,10 +76,13 @@ async function isFeedbackInvokeAuthorized( return true; } + const maybeInvokeName = includeInvokeName ? { name: context.activity.name } : undefined; + if (isDirectMessage && resolved.access.decision !== "allow") { - deps.log.debug?.("dropping feedback invoke (dm sender not allowlisted)", { + deps.log.debug?.(deniedLogs.dm, { sender: senderId, conversationId, + ...maybeInvokeName, }); return false; } @@ -82,18 +92,20 @@ async function isFeedbackInvokeAuthorized( resolved.channelGate.allowlistConfigured && !resolved.channelGate.allowed ) { - deps.log.debug?.("dropping feedback invoke (not in team/channel allowlist)", { + deps.log.debug?.(deniedLogs.channel, { conversationId, teamKey: resolved.channelGate.teamKey ?? "none", channelKey: resolved.channelGate.channelKey ?? "none", + ...maybeInvokeName, }); return false; } if (!isDirectMessage && !resolved.senderGroupAccess.allowed) { - deps.log.debug?.("dropping feedback invoke (group sender not allowlisted)", { + deps.log.debug?.(deniedLogs.group, { sender: senderId, conversationId, + ...maybeInvokeName, }); return false; } @@ -101,6 +113,37 @@ async function isFeedbackInvokeAuthorized( return true; } +async function isFeedbackInvokeAuthorized( + context: MSTeamsTurnContext, + deps: MSTeamsMessageHandlerDeps, +): Promise { + return isInvokeAuthorized({ + context, + deps, + deniedLogs: { + dm: "dropping feedback invoke (dm sender not allowlisted)", + channel: "dropping feedback invoke (not in team/channel allowlist)", + group: "dropping feedback invoke (group sender not allowlisted)", + }, + }); +} + +async function isSigninInvokeAuthorized( + context: MSTeamsTurnContext, + deps: MSTeamsMessageHandlerDeps, +): Promise { + return isInvokeAuthorized({ + context, + deps, + deniedLogs: { + dm: "dropping signin invoke (dm sender not allowlisted)", + channel: "dropping signin invoke (not in team/channel allowlist)", + group: "dropping signin invoke (group sender not allowlisted)", + }, + includeInvokeName: true, + }); +} + /** * Handle fileConsent/invoke activities for large file uploads. */ @@ -481,6 +524,10 @@ export function registerMSTeamsHandlers( // the Teams card UI to report "Something went wrong". await ctx.sendActivity({ type: "invokeResponse", value: { status: 200, body: {} } }); + if (!(await isSigninInvokeAuthorized(ctx, deps))) { + return; + } + if (!deps.sso) { deps.log.debug?.("signin invoke received but msteams.sso is not configured", { name: ctx.activity.name,