diff --git a/CHANGELOG.md b/CHANGELOG.md index 49b5aa90b84..e15f497498d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -172,6 +172,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(msteams): gate startup user allowlist resolution [AI]. (#79003) Thanks @pgondhi987. - Harden macOS shell wrapper allowlist parsing [AI]. (#78518) Thanks @pgondhi987. - Gateway/macOS: `openclaw gateway stop` now uses `launchctl bootout` by default instead of unconditionally calling `launchctl disable`, so KeepAlive auto-recovery still works after unexpected crashes; use the new `--disable` flag to opt into the persistent-disable behavior when a manual stop should survive reboots. Fixes #77934. Thanks @bmoran1022. - Gateway/macOS: `repairLaunchAgentBootstrap` no longer kickstarts an already-running LaunchAgent, preventing unnecessary service restarts and session disconnects when repair runs against a healthy gateway. Fixes #77428. Thanks @ramitrkar-hash. diff --git a/extensions/msteams/src/monitor.lifecycle.test.ts b/extensions/msteams/src/monitor.lifecycle.test.ts index f9d9bf4e917..e110bdb8e0e 100644 --- a/extensions/msteams/src/monitor.lifecycle.test.ts +++ b/extensions/msteams/src/monitor.lifecycle.test.ts @@ -21,8 +21,11 @@ const expressControl = vi.hoisted(() => ({ }>, })); +const isDangerousNameMatchingEnabled = vi.hoisted(() => vi.fn()); + vi.mock("../runtime-api.js", () => ({ DEFAULT_WEBHOOK_MAX_BODY_BYTES: 1024 * 1024, + isDangerousNameMatchingEnabled, normalizeSecretInputString: (value: unknown) => typeof value === "string" && value.trim() ? value.trim() : undefined, hasConfiguredSecretInput: (value: unknown) => @@ -115,12 +118,17 @@ const loadMSTeamsSdkWithAuth = vi.hoisted(() => ); vi.mock("./monitor-handler.js", () => ({ - registerMSTeamsHandlers: () => registerMSTeamsHandlers(), + registerMSTeamsHandlers: (...args: unknown[]) => registerMSTeamsHandlers(...args), +})); + +const resolveAllowlistMocks = vi.hoisted(() => ({ + resolveMSTeamsChannelAllowlist: vi.fn(async () => []), + resolveMSTeamsUserAllowlist: vi.fn(async () => []), })); vi.mock("./resolve-allowlist.js", () => ({ - resolveMSTeamsChannelAllowlist: vi.fn(async () => []), - resolveMSTeamsUserAllowlist: vi.fn(async () => []), + resolveMSTeamsChannelAllowlist: resolveAllowlistMocks.resolveMSTeamsChannelAllowlist, + resolveMSTeamsUserAllowlist: resolveAllowlistMocks.resolveMSTeamsUserAllowlist, })); vi.mock("./sdk.js", () => ({ @@ -192,6 +200,9 @@ describe("monitorMSTeamsProvider lifecycle", () => { vi.clearAllMocks(); expressControl.mode.value = "listening"; expressControl.apps.length = 0; + isDangerousNameMatchingEnabled.mockReset().mockReturnValue(false); + resolveAllowlistMocks.resolveMSTeamsChannelAllowlist.mockReset().mockResolvedValue([]); + resolveAllowlistMocks.resolveMSTeamsUserAllowlist.mockReset().mockResolvedValue([]); jwtValidate.mockReset().mockResolvedValue(true); }); @@ -277,4 +288,106 @@ describe("monitorMSTeamsProvider lifecycle", () => { abort.abort(); await task; }); + + it("does not resolve user allowlists by display name unless name matching is enabled", async () => { + const abort = new AbortController(); + const cfg = createConfig(0); + cfg.channels!.msteams = { + ...cfg.channels!.msteams!, + allowFrom: ["Alice", "user:40a1a0ed-4ff2-4164-a219-55518990c197"], + groupAllowFrom: ["Bob", "msteams:user:50a1a0ed-4ff2-4164-a219-55518990c198"], + teams: { + Product: { + channels: { + Roadmap: {}, + }, + }, + }, + }; + resolveAllowlistMocks.resolveMSTeamsChannelAllowlist.mockResolvedValueOnce([ + { + input: "Product/Roadmap", + resolved: true, + teamId: "team-id", + channelId: "channel-id", + }, + ]); + + const task = monitorMSTeamsProvider({ + cfg, + runtime: createRuntime(), + abortSignal: abort.signal, + conversationStore: createStores().conversationStore, + pollStore: createStores().pollStore, + }); + + await vi.waitFor(() => { + expect(registerMSTeamsHandlers).toHaveBeenCalled(); + }); + + expect(resolveAllowlistMocks.resolveMSTeamsUserAllowlist).not.toHaveBeenCalled(); + expect(resolveAllowlistMocks.resolveMSTeamsChannelAllowlist).toHaveBeenCalledWith({ + cfg, + entries: ["Product/Roadmap"], + }); + + const registeredCfg = registerMSTeamsHandlers.mock.calls[0]?.[1] as { cfg: OpenClawConfig }; + expect(registeredCfg.cfg.channels?.msteams?.allowFrom).toEqual([ + "Alice", + "user:40a1a0ed-4ff2-4164-a219-55518990c197", + "40a1a0ed-4ff2-4164-a219-55518990c197", + ]); + expect(registeredCfg.cfg.channels?.msteams?.groupAllowFrom).toEqual([ + "Bob", + "msteams:user:50a1a0ed-4ff2-4164-a219-55518990c198", + "50a1a0ed-4ff2-4164-a219-55518990c198", + ]); + + abort.abort(); + await task; + }); + + it("resolves user allowlists when name matching is enabled", async () => { + isDangerousNameMatchingEnabled.mockReturnValue(true); + resolveAllowlistMocks.resolveMSTeamsUserAllowlist + .mockResolvedValueOnce([{ input: "Alice", resolved: true, id: "alice-aad" }]) + .mockResolvedValueOnce([{ input: "Bob", resolved: true, id: "bob-aad" }]); + + const abort = new AbortController(); + const cfg = createConfig(0); + cfg.channels!.msteams = { + ...cfg.channels!.msteams!, + dangerouslyAllowNameMatching: true, + allowFrom: ["Alice"], + groupAllowFrom: ["Bob"], + }; + + const task = monitorMSTeamsProvider({ + cfg, + runtime: createRuntime(), + abortSignal: abort.signal, + conversationStore: createStores().conversationStore, + pollStore: createStores().pollStore, + }); + + await vi.waitFor(() => { + expect(registerMSTeamsHandlers).toHaveBeenCalled(); + }); + + expect(resolveAllowlistMocks.resolveMSTeamsUserAllowlist).toHaveBeenNthCalledWith(1, { + cfg, + entries: ["Alice"], + }); + expect(resolveAllowlistMocks.resolveMSTeamsUserAllowlist).toHaveBeenNthCalledWith(2, { + cfg, + entries: ["Bob"], + }); + + const registeredCfg = registerMSTeamsHandlers.mock.calls[0]?.[1] as { cfg: OpenClawConfig }; + expect(registeredCfg.cfg.channels?.msteams?.allowFrom).toEqual(["Alice", "alice-aad"]); + expect(registeredCfg.cfg.channels?.msteams?.groupAllowFrom).toEqual(["Bob", "bob-aad"]); + + abort.abort(); + await task; + }); }); diff --git a/extensions/msteams/src/monitor.ts b/extensions/msteams/src/monitor.ts index 6c1c5ceb9e7..a2ecb2aa665 100644 --- a/extensions/msteams/src/monitor.ts +++ b/extensions/msteams/src/monitor.ts @@ -1,6 +1,7 @@ import type { Request, Response } from "express"; import { DEFAULT_WEBHOOK_MAX_BODY_BYTES, + isDangerousNameMatchingEnabled, keepHttpServerTaskAlive, mergeAllowlist, summarizeMapping, @@ -73,12 +74,20 @@ export async function monitorMSTeamsProvider( let allowFrom = msteamsCfg.allowFrom; let groupAllowFrom = msteamsCfg.groupAllowFrom; let teamsConfig = msteamsCfg.teams; + const allowNameMatching = isDangerousNameMatchingEnabled(msteamsCfg); const cleanAllowEntry = (entry: string) => entry .replace(/^(msteams|teams):/i, "") .replace(/^user:/i, "") .trim(); + const isStableUserId = (entry: string) => /^[0-9a-fA-F-]{16,}$/.test(entry); + const cleanAllowEntries = (entries?: string[]) => + entries?.map((entry) => cleanAllowEntry(entry)).filter((entry) => entry && entry !== "*") ?? []; + const mergeStableUserIds = (entries?: string[]) => { + const additions = cleanAllowEntries(entries).filter((entry) => isStableUserId(entry)); + return additions.length > 0 ? mergeAllowlist({ existing: entries, additions }) : entries; + }; const resolveAllowlistUsers = async (label: string, entries: string[]) => { if (entries.length === 0) { @@ -102,21 +111,26 @@ export async function monitorMSTeamsProvider( }; try { - const allowEntries = - allowFrom?.map((entry) => cleanAllowEntry(entry)).filter((entry) => entry && entry !== "*") ?? - []; - if (allowEntries.length > 0) { - const { additions } = await resolveAllowlistUsers("msteams users", allowEntries); - allowFrom = mergeAllowlist({ existing: allowFrom, additions }); + allowFrom = mergeStableUserIds(allowFrom); + if (Array.isArray(groupAllowFrom) && groupAllowFrom.length > 0) { + groupAllowFrom = mergeStableUserIds(groupAllowFrom); } - if (Array.isArray(groupAllowFrom) && groupAllowFrom.length > 0) { - const groupEntries = groupAllowFrom - .map((entry) => cleanAllowEntry(entry)) - .filter((entry) => entry && entry !== "*"); - if (groupEntries.length > 0) { - const { additions } = await resolveAllowlistUsers("msteams group users", groupEntries); - groupAllowFrom = mergeAllowlist({ existing: groupAllowFrom, additions }); + if (allowNameMatching) { + const allowEntries = cleanAllowEntries(allowFrom).filter((entry) => !isStableUserId(entry)); + if (allowEntries.length > 0) { + const { additions } = await resolveAllowlistUsers("msteams users", allowEntries); + allowFrom = mergeAllowlist({ existing: allowFrom, additions }); + } + + if (Array.isArray(groupAllowFrom) && groupAllowFrom.length > 0) { + const groupEntries = cleanAllowEntries(groupAllowFrom).filter( + (entry) => !isStableUserId(entry), + ); + if (groupEntries.length > 0) { + const { additions } = await resolveAllowlistUsers("msteams group users", groupEntries); + groupAllowFrom = mergeAllowlist({ existing: groupAllowFrom, additions }); + } } }