mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 06:10:44 +00:00
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
This commit is contained in:
committed by
GitHub
parent
75b4c059b8
commit
80b1fa17bf
@@ -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
|
||||
|
||||
|
||||
@@ -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<typeof vi.fn> } {
|
||||
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<MSTeamsActivityHandler["run"]>;
|
||||
};
|
||||
|
||||
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([
|
||||
() => ({
|
||||
|
||||
@@ -56,10 +56,17 @@ function serializeAdaptiveCardActionValue(value: unknown): string | null {
|
||||
}
|
||||
}
|
||||
|
||||
async function isFeedbackInvokeAuthorized(
|
||||
context: MSTeamsTurnContext,
|
||||
deps: MSTeamsMessageHandlerDeps,
|
||||
): Promise<boolean> {
|
||||
async function isInvokeAuthorized(params: {
|
||||
context: MSTeamsTurnContext;
|
||||
deps: MSTeamsMessageHandlerDeps;
|
||||
deniedLogs: {
|
||||
dm: string;
|
||||
channel: string;
|
||||
group: string;
|
||||
};
|
||||
includeInvokeName?: boolean;
|
||||
}): Promise<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<T extends MSTeamsActivityHandler>(
|
||||
// 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,
|
||||
|
||||
Reference in New Issue
Block a user