From c56b56e514f804561075356e22b8ec1ab4893bee Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 15 Apr 2026 20:30:23 -0700 Subject: [PATCH] fix(msteams): harden security-sensitive flows (#65841) * fix(msteams): validate participant graph params * fix(msteams): restore media fetch ip guard * fix(msteams): open delegated auth urls without shell --- extensions/msteams/src/attachments.test.ts | 2 + .../msteams/src/attachments/bot-framework.ts | 10 ++++ .../msteams/src/attachments/download.ts | 6 +++ extensions/msteams/src/attachments/graph.ts | 4 ++ .../msteams/src/attachments/shared.test.ts | 12 +++++ extensions/msteams/src/attachments/shared.ts | 10 ++-- .../src/graph-group-management.test.ts | 47 +++++++++++++++++++ .../msteams/src/graph-group-management.ts | 18 ++++++- extensions/msteams/src/setup-surface.test.ts | 27 +++++++++++ extensions/msteams/src/setup-surface.ts | 24 +++++++--- 10 files changed, 148 insertions(+), 12 deletions(-) diff --git a/extensions/msteams/src/attachments.test.ts b/extensions/msteams/src/attachments.test.ts index cb78b4264b7..1a34842a7f7 100644 --- a/extensions/msteams/src/attachments.test.ts +++ b/extensions/msteams/src/attachments.test.ts @@ -208,6 +208,7 @@ const _createGraphCollectionResponse = (value: unknown[]) => createJsonResponse( const createNotFoundResponse = () => new Response("not found", { status: 404 }); const createRedirectResponse = (location: string, status = 302) => new Response(null, { status, headers: { location } }); +const publicResolve = async () => ({ address: "13.107.136.10" }); const createOkFetchMock = (contentType: string, payload = "png") => vi.fn(async (_input: RequestInfo | URL, _init?: RequestInit) => @@ -223,6 +224,7 @@ const buildDownloadParams = ( attachments, maxBytes: DEFAULT_MAX_BYTES, allowHosts: DEFAULT_ALLOW_HOSTS, + resolveFn: publicResolve, ...overrides, }; }; diff --git a/extensions/msteams/src/attachments/bot-framework.ts b/extensions/msteams/src/attachments/bot-framework.ts index db2be0ae072..7ec8ce43b34 100644 --- a/extensions/msteams/src/attachments/bot-framework.ts +++ b/extensions/msteams/src/attachments/bot-framework.ts @@ -6,6 +6,7 @@ import { isUrlAllowed, type MSTeamsAttachmentDownloadLogger, type MSTeamsAttachmentFetchPolicy, + type MSTeamsAttachmentResolveFn, resolveAttachmentFetchPolicy, safeFetchWithPolicy, } from "./shared.js"; @@ -59,6 +60,7 @@ async function fetchBotFrameworkAttachmentInfo(params: { accessToken: string; policy: MSTeamsAttachmentFetchPolicy; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; logger?: MSTeamsAttachmentDownloadLogger; }): Promise { const url = `${normalizeServiceUrl(params.serviceUrl)}/v3/attachments/${encodeURIComponent(params.attachmentId)}`; @@ -75,6 +77,7 @@ async function fetchBotFrameworkAttachmentInfo(params: { url, policy: params.policy, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, requestInit: { headers: ensureUserAgentHeader({ Authorization: `Bearer ${params.accessToken}` }), }, @@ -109,6 +112,7 @@ async function fetchBotFrameworkAttachmentView(params: { maxBytes: number; policy: MSTeamsAttachmentFetchPolicy; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; logger?: MSTeamsAttachmentDownloadLogger; }): Promise { const url = `${normalizeServiceUrl(params.serviceUrl)}/v3/attachments/${encodeURIComponent(params.attachmentId)}/views/${encodeURIComponent(params.viewId)}`; @@ -120,6 +124,7 @@ async function fetchBotFrameworkAttachmentView(params: { url, policy: params.policy, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, requestInit: { headers: ensureUserAgentHeader({ Authorization: `Bearer ${params.accessToken}` }), }, @@ -169,6 +174,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: { allowHosts?: string[]; authAllowHosts?: string[]; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; fileNameHint?: string | null; contentTypeHint?: string | null; preserveFilenames?: boolean; @@ -205,6 +211,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: { accessToken, policy, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, logger: params.logger, }); if (!info) { @@ -239,6 +246,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: { maxBytes: params.maxBytes, policy, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, logger: params.logger, }); if (!buffer) { @@ -296,6 +304,7 @@ export async function downloadMSTeamsBotFrameworkAttachments(params: { allowHosts?: string[]; authAllowHosts?: string[]; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; fileNameHint?: string | null; contentTypeHint?: string | null; preserveFilenames?: boolean; @@ -329,6 +338,7 @@ export async function downloadMSTeamsBotFrameworkAttachments(params: { allowHosts: params.allowHosts, authAllowHosts: params.authAllowHosts, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, fileNameHint: params.fileNameHint, contentTypeHint: params.contentTypeHint, preserveFilenames: params.preserveFilenames, diff --git a/extensions/msteams/src/attachments/download.ts b/extensions/msteams/src/attachments/download.ts index 8de92434fd5..918309a8055 100644 --- a/extensions/msteams/src/attachments/download.ts +++ b/extensions/msteams/src/attachments/download.ts @@ -12,6 +12,7 @@ import { isUrlAllowed, type MSTeamsAttachmentDownloadLogger, type MSTeamsAttachmentFetchPolicy, + type MSTeamsAttachmentResolveFn, normalizeContentType, resolveMediaSsrfPolicy, resolveAttachmentFetchPolicy, @@ -111,6 +112,7 @@ async function fetchWithAuthFallback(params: { tokenProvider?: MSTeamsAccessTokenProvider; fetchFn?: typeof fetch; requestInit?: RequestInit; + resolveFn?: MSTeamsAttachmentResolveFn; policy: MSTeamsAttachmentFetchPolicy; }): Promise { const firstAttempt = await safeFetchWithPolicy({ @@ -118,6 +120,7 @@ async function fetchWithAuthFallback(params: { policy: params.policy, fetchFn: params.fetchFn, requestInit: params.requestInit, + resolveFn: params.resolveFn, }); if (firstAttempt.ok) { return firstAttempt; @@ -147,6 +150,7 @@ async function fetchWithAuthFallback(params: { ...params.requestInit, headers: authHeaders, }, + resolveFn: params.resolveFn, }); if (authAttempt.ok) { return authAttempt; @@ -178,6 +182,7 @@ export async function downloadMSTeamsAttachments(params: { allowHosts?: string[]; authAllowHosts?: string[]; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; /** When true, embeds original filename in stored path for later extraction. */ preserveFilenames?: boolean; /** @@ -282,6 +287,7 @@ export async function downloadMSTeamsAttachments(params: { tokenProvider: params.tokenProvider, fetchFn: params.fetchFn, requestInit: init, + resolveFn: params.resolveFn, policy, }), }); diff --git a/extensions/msteams/src/attachments/graph.ts b/extensions/msteams/src/attachments/graph.ts index 95651254d02..86d6247a0d7 100644 --- a/extensions/msteams/src/attachments/graph.ts +++ b/extensions/msteams/src/attachments/graph.ts @@ -18,6 +18,7 @@ import { isUrlAllowed, type MSTeamsAttachmentDownloadLogger, type MSTeamsAttachmentFetchPolicy, + type MSTeamsAttachmentResolveFn, normalizeContentType, resolveMediaSsrfPolicy, resolveAttachmentFetchPolicy, @@ -280,6 +281,7 @@ export async function downloadMSTeamsGraphMedia(params: { allowHosts?: string[]; authAllowHosts?: string[]; fetchFn?: typeof fetch; + resolveFn?: MSTeamsAttachmentResolveFn; /** When true, embeds original filename in stored path for later extraction. */ preserveFilenames?: boolean; /** Optional logger used to surface Graph/SharePoint fetch errors. */ @@ -394,6 +396,7 @@ export async function downloadMSTeamsGraphMedia(params: { ...init, headers, }, + resolveFn: params.resolveFn, }); }, }); @@ -459,6 +462,7 @@ export async function downloadMSTeamsGraphMedia(params: { allowHosts: policy.allowHosts, authAllowHosts: policy.authAllowHosts, fetchFn: params.fetchFn, + resolveFn: params.resolveFn, preserveFilenames: params.preserveFilenames, logger: params.logger, }); diff --git a/extensions/msteams/src/attachments/shared.test.ts b/extensions/msteams/src/attachments/shared.test.ts index 7e83b7d983f..06a415b36c7 100644 --- a/extensions/msteams/src/attachments/shared.test.ts +++ b/extensions/msteams/src/attachments/shared.test.ts @@ -254,6 +254,18 @@ describe("safeFetch", () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it("blocks private hosts with the default resolver", async () => { + const fetchMock = vi.fn(); + await expect( + safeFetch({ + url: "https://localhost/file.pdf", + allowHosts: ["localhost"], + fetchFn: fetchMock as unknown as typeof fetch, + }), + ).rejects.toThrow("Initial download URL blocked"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it("blocks when initial URL DNS resolution fails", async () => { const fetchMock = vi.fn(); await expect( diff --git a/extensions/msteams/src/attachments/shared.ts b/extensions/msteams/src/attachments/shared.ts index 93630338863..1a9e839f71b 100644 --- a/extensions/msteams/src/attachments/shared.ts +++ b/extensions/msteams/src/attachments/shared.ts @@ -402,6 +402,8 @@ export type MSTeamsAttachmentDownloadLogger = { error?: (message: string, meta?: Record) => void; }; +export type MSTeamsAttachmentResolveFn = (hostname: string) => Promise<{ address: string }>; + export function resolveAttachmentFetchPolicy(params?: { allowHosts?: string[]; authAllowHosts?: string[]; @@ -453,7 +455,7 @@ export const isPrivateOrReservedIP: (ip: string) => boolean = isPrivateIpAddress */ export async function resolveAndValidateIP( hostname: string, - resolveFn?: (hostname: string) => Promise<{ address: string }>, + resolveFn?: MSTeamsAttachmentResolveFn, ): Promise { const resolve = resolveFn ?? lookup; let resolved: { address: string }; @@ -490,10 +492,10 @@ export async function safeFetch(params: { authorizationAllowHosts?: string[]; fetchFn?: typeof fetch; requestInit?: RequestInit; - resolveFn?: (hostname: string) => Promise<{ address: string }>; + resolveFn?: MSTeamsAttachmentResolveFn; }): Promise { const fetchFn = params.fetchFn ?? fetch; - const resolveFn = params.resolveFn; + const resolveFn = params.resolveFn ?? lookup; const hasDispatcher = Boolean( params.requestInit && typeof params.requestInit === "object" && @@ -577,7 +579,7 @@ export async function safeFetchWithPolicy(params: { policy: MSTeamsAttachmentFetchPolicy; fetchFn?: typeof fetch; requestInit?: RequestInit; - resolveFn?: (hostname: string) => Promise<{ address: string }>; + resolveFn?: MSTeamsAttachmentResolveFn; }): Promise { return await safeFetch({ url: params.url, diff --git a/extensions/msteams/src/graph-group-management.test.ts b/extensions/msteams/src/graph-group-management.test.ts index 47b150a0c8a..84c2d9d7147 100644 --- a/extensions/msteams/src/graph-group-management.test.ts +++ b/extensions/msteams/src/graph-group-management.test.ts @@ -86,6 +86,38 @@ describe("addParticipantMSTeams", () => { }); }); + it("normalizes role casing and whitespace", async () => { + mockState.postGraphJson.mockResolvedValue({}); + + await addParticipantMSTeams({ + cfg: {} as OpenClawConfig, + to: CHAT_ID, + userId: "user-aad-id-2", + role: " OWNER ", + }); + + expect(mockState.postGraphJson).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.objectContaining({ + roles: ["owner"], + }), + }), + ); + }); + + it("rejects unknown roles", async () => { + await expect( + addParticipantMSTeams({ + cfg: {} as OpenClawConfig, + to: CHAT_ID, + userId: "user-aad-id-2", + role: "admin", + }), + ).rejects.toThrow('role must be "member" or "owner"'); + + expect(mockState.postGraphJson).not.toHaveBeenCalled(); + }); + it("constructs correct user@odata.bind URL", async () => { mockState.postGraphJson.mockResolvedValue({}); @@ -101,6 +133,21 @@ describe("addParticipantMSTeams", () => { ); }); + it("escapes user ids before building the OData bind URL", async () => { + mockState.postGraphJson.mockResolvedValue({}); + + await addParticipantMSTeams({ + cfg: {} as OpenClawConfig, + to: CHAT_ID, + userId: "o'hara@example.com", + }); + + const calledBody = mockState.postGraphJson.mock.calls[0][0].body; + expect(calledBody["user@odata.bind"]).toBe( + "https://graph.microsoft.com/v1.0/users('o''hara@example.com')", + ); + }); + it("adds member to a channel", async () => { mockState.postGraphJson.mockResolvedValue({}); diff --git a/extensions/msteams/src/graph-group-management.ts b/extensions/msteams/src/graph-group-management.ts index ed182ad006a..3a3fd3071b6 100644 --- a/extensions/msteams/src/graph-group-management.ts +++ b/extensions/msteams/src/graph-group-management.ts @@ -2,6 +2,7 @@ import type { OpenClawConfig } from "../runtime-api.js"; import { resolveConversationPath, resolveGraphConversationId } from "./graph-messages.js"; import { deleteGraphRequest, + escapeOData, fetchGraphJson, patchGraphJson, postGraphJson, @@ -23,6 +24,19 @@ export type AddParticipantMSTeamsResult = { added: { userId: string; chatId: string }; }; +type ConversationMemberRole = "member" | "owner"; + +function normalizeConversationMemberRole(role: string | undefined): ConversationMemberRole { + const normalized = role?.trim().toLowerCase() ?? ""; + if (!normalized) { + return "member"; + } + if (normalized === "member" || normalized === "owner") { + return normalized; + } + throw new Error('MS Teams participant role must be "member" or "owner".'); +} + /** * Add a user to a chat or channel via Graph API. */ @@ -35,8 +49,8 @@ export async function addParticipantMSTeams( const body = { "@odata.type": "#microsoft.graph.aadUserConversationMember", - roles: [params.role || "member"], - "user@odata.bind": `https://graph.microsoft.com/v1.0/users('${params.userId}')`, + roles: [normalizeConversationMemberRole(params.role)], + "user@odata.bind": `https://graph.microsoft.com/v1.0/users('${escapeOData(params.userId)}')`, }; await postGraphJson({ diff --git a/extensions/msteams/src/setup-surface.test.ts b/extensions/msteams/src/setup-surface.test.ts index a3effc8a445..85cd8d45a07 100644 --- a/extensions/msteams/src/setup-surface.test.ts +++ b/extensions/msteams/src/setup-surface.test.ts @@ -1,7 +1,10 @@ +import { EventEmitter } from "node:events"; import { DEFAULT_ACCOUNT_ID } from "openclaw/plugin-sdk/setup"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createMSTeamsSetupWizardBase, msteamsSetupAdapter } from "./setup-core.js"; +import { openDelegatedOAuthUrl } from "./setup-surface.js"; +const spawn = vi.hoisted(() => vi.fn()); const resolveMSTeamsUserAllowlist = vi.hoisted(() => vi.fn()); const resolveMSTeamsChannelAllowlist = vi.hoisted(() => vi.fn()); const normalizeSecretInputString = vi.hoisted(() => @@ -25,10 +28,19 @@ vi.mock("./token.js", () => ({ resolveMSTeamsCredentials, })); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn, + }; +}); + describe("msteams setup surface", () => { const msteamsSetupWizard = createMSTeamsSetupWizardBase(); beforeEach(() => { + spawn.mockReset(); resolveMSTeamsUserAllowlist.mockReset(); resolveMSTeamsChannelAllowlist.mockReset(); normalizeSecretInputString.mockClear(); @@ -46,6 +58,21 @@ describe("msteams setup surface", () => { ); }); + it("opens delegated OAuth URLs without invoking a shell", async () => { + const url = "https://login.microsoftonline.com/auth?state=$(touch pwned)"; + const child = new EventEmitter(); + spawn.mockReturnValue(child); + + const result = openDelegatedOAuthUrl(url); + child.emit("exit", 0, null); + + await expect(result).resolves.toBeUndefined(); + expect(spawn).toHaveBeenCalledWith(process.platform === "darwin" ? "open" : "xdg-open", [url], { + stdio: "ignore", + shell: false, + }); + }); + it("enables the msteams channel without dropping existing config", () => { expect( msteamsSetupAdapter.applyAccountConfig?.({ diff --git a/extensions/msteams/src/setup-surface.ts b/extensions/msteams/src/setup-surface.ts index b65824c0546..d35c315a735 100644 --- a/extensions/msteams/src/setup-surface.ts +++ b/extensions/msteams/src/setup-surface.ts @@ -1,4 +1,4 @@ -import { exec } from "node:child_process"; +import { spawn } from "node:child_process"; import { createTopLevelChannelAllowFromSetter, createTopLevelChannelDmPolicy, @@ -29,6 +29,22 @@ const setMSTeamsGroupPolicy = createTopLevelChannelGroupPolicySetter({ enabled: true, }); +export function openDelegatedOAuthUrl(url: string): Promise { + return new Promise((resolve, reject) => { + const cmd = process.platform === "darwin" ? "open" : "xdg-open"; + const child = spawn(cmd, [url], { stdio: "ignore", shell: false }); + child.once("error", reject); + child.once("exit", (code, signal) => { + if (code === 0) { + resolve(); + return; + } + const reason = signal ? `signal ${signal}` : `code ${code ?? "unknown"}`; + reject(new Error(`${cmd} failed with ${reason}`)); + }); + }); +} + function looksLikeGuid(value: string): boolean { return /^[0-9a-fA-F-]{16,}$/.test(value); } @@ -270,11 +286,7 @@ export const msteamsSetupWizard: ChannelSetupWizard = { const tokens = await loginMSTeamsDelegated( { isRemote: shouldUseManualOAuthFlow(isRemote), - openUrl: (url) => - new Promise((resolve, reject) => { - const cmd = process.platform === "darwin" ? "open" : "xdg-open"; - exec(`${cmd} ${JSON.stringify(url)}`, (err) => (err ? reject(err) : resolve())); - }), + openUrl: openDelegatedOAuthUrl, log: (msg) => params.prompter.note(msg), note: (msg, title) => params.prompter.note(msg, title), prompt: (msg) => params.prompter.text({ message: msg }),