From 27dad962fee567e50ed7e293a4d8eb67cc7da8e6 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 7 Mar 2026 21:50:19 +0000 Subject: [PATCH] refactor: normalize runtime group sender gating decisions --- .../mattermost/src/mattermost/monitor-auth.ts | 69 +++++++++------- .../src/monitor-handler/message-handler.ts | 82 +++++++++++-------- src/line/bot-handlers.ts | 37 +++++---- src/plugin-sdk/group-access.test.ts | 30 ++++++- src/plugin-sdk/group-access.ts | 78 +++++++++++------- src/plugin-sdk/index.ts | 1 + src/plugin-sdk/mattermost.ts | 1 + src/plugin-sdk/msteams.ts | 1 + 8 files changed, 190 insertions(+), 109 deletions(-) diff --git a/extensions/mattermost/src/mattermost/monitor-auth.ts b/extensions/mattermost/src/mattermost/monitor-auth.ts index 530502f9101..7f263cd09b5 100644 --- a/extensions/mattermost/src/mattermost/monitor-auth.ts +++ b/extensions/mattermost/src/mattermost/monitor-auth.ts @@ -1,5 +1,6 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/mattermost"; import { + evaluateSenderGroupAccessForPolicy, isDangerousNameMatchingEnabled, resolveAllowlistMatchSimple, resolveControlCommandGate, @@ -231,7 +232,20 @@ export function authorizeMattermostCommandInvocation(params: { }; } } else { - if (groupPolicy === "disabled") { + const senderGroupAccess = evaluateSenderGroupAccessForPolicy({ + groupPolicy, + groupAllowFrom: effectiveGroupAllowFrom, + senderId, + isSenderAllowed: (_senderId, allowFrom) => + isMattermostSenderAllowed({ + senderId, + senderName, + allowFrom, + allowNameMatching, + }), + }); + + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") { return { ok: false, denyReason: "channels-disabled", @@ -245,33 +259,32 @@ export function authorizeMattermostCommandInvocation(params: { }; } - if (groupPolicy === "allowlist") { - if (effectiveGroupAllowFrom.length === 0) { - return { - ok: false, - denyReason: "channel-no-allowlist", - commandAuthorized: false, - channelInfo, - kind, - chatType, - channelName, - channelDisplay, - roomLabel, - }; - } - if (!groupAllowedForCommands) { - return { - ok: false, - denyReason: "unauthorized", - commandAuthorized: false, - channelInfo, - kind, - chatType, - channelName, - channelDisplay, - roomLabel, - }; - } + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") { + return { + ok: false, + denyReason: "channel-no-allowlist", + commandAuthorized: false, + channelInfo, + kind, + chatType, + channelName, + channelDisplay, + roomLabel, + }; + } + + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") { + return { + ok: false, + denyReason: "unauthorized", + commandAuthorized: false, + channelInfo, + kind, + chatType, + channelName, + channelDisplay, + roomLabel, + }; } if (commandGate.shouldBlock) { diff --git a/extensions/msteams/src/monitor-handler/message-handler.ts b/extensions/msteams/src/monitor-handler/message-handler.ts index ba68fc9f5c9..e4a88eb3b6a 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.ts @@ -6,6 +6,7 @@ import { DEFAULT_GROUP_HISTORY_LIMIT, createScopedPairingAccess, logInboundDrop, + evaluateSenderGroupAccessForPolicy, recordPendingHistoryEntryIfEnabled, resolveControlCommandGate, resolveDefaultGroupPolicy, @@ -230,46 +231,57 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { } if (!isDirectMessage && msteamsCfg) { - if (groupPolicy === "disabled") { + if (channelGate.allowlistConfigured && !channelGate.allowed) { + log.debug?.("dropping group message (not in team/channel allowlist)", { + conversationId, + teamKey: channelGate.teamKey ?? "none", + channelKey: channelGate.channelKey ?? "none", + channelMatchKey: channelGate.channelMatchKey ?? "none", + channelMatchSource: channelGate.channelMatchSource ?? "none", + }); + return; + } + const senderGroupAccess = evaluateSenderGroupAccessForPolicy({ + groupPolicy, + groupAllowFrom: + effectiveGroupAllowFrom.length > 0 || !channelGate.allowlistConfigured + ? effectiveGroupAllowFrom + : ["*"], + senderId, + isSenderAllowed: (_senderId, allowFrom) => + resolveMSTeamsAllowlistMatch({ + allowFrom, + senderId, + senderName, + allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg), + }).allowed, + }); + + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") { log.debug?.("dropping group message (groupPolicy: disabled)", { conversationId, }); return; } - - if (groupPolicy === "allowlist") { - if (channelGate.allowlistConfigured && !channelGate.allowed) { - log.debug?.("dropping group message (not in team/channel allowlist)", { - conversationId, - teamKey: channelGate.teamKey ?? "none", - channelKey: channelGate.channelKey ?? "none", - channelMatchKey: channelGate.channelMatchKey ?? "none", - channelMatchSource: channelGate.channelMatchSource ?? "none", - }); - return; - } - if (effectiveGroupAllowFrom.length === 0 && !channelGate.allowlistConfigured) { - log.debug?.("dropping group message (groupPolicy: allowlist, no allowlist)", { - conversationId, - }); - return; - } - if (effectiveGroupAllowFrom.length > 0 && access.decision !== "allow") { - const allowMatch = resolveMSTeamsAllowlistMatch({ - allowFrom: effectiveGroupAllowFrom, - senderId, - senderName, - allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg), - }); - if (!allowMatch.allowed) { - log.debug?.("dropping group message (not in groupAllowFrom)", { - sender: senderId, - label: senderName, - allowlistMatch: formatAllowlistMatchMeta(allowMatch), - }); - return; - } - } + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") { + log.debug?.("dropping group message (groupPolicy: allowlist, no allowlist)", { + conversationId, + }); + return; + } + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") { + const allowMatch = resolveMSTeamsAllowlistMatch({ + allowFrom: effectiveGroupAllowFrom, + senderId, + senderName, + allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg), + }); + log.debug?.("dropping group message (not in groupAllowFrom)", { + sender: senderId, + label: senderName, + allowlistMatch: formatAllowlistMatchMeta(allowMatch), + }); + return; } } diff --git a/src/line/bot-handlers.ts b/src/line/bot-handlers.ts index 2772965a8e6..479cf0781e4 100644 --- a/src/line/bot-handlers.ts +++ b/src/line/bot-handlers.ts @@ -30,6 +30,7 @@ import { readChannelAllowFromStore, upsertChannelPairingRequest, } from "../pairing/pairing-store.js"; +import { evaluateSenderGroupAccessForPolicy } from "../plugin-sdk/group-access.js"; import { resolveAgentRoute } from "../routing/resolve-route.js"; import type { RuntimeEnv } from "../runtime.js"; import { @@ -344,23 +345,31 @@ async function shouldProcessLineEvent( return denied; } } - if (groupPolicy === "disabled") { + if (groupPolicy === "allowlist" && !senderId) { + logVerbose("Blocked line group message (no sender ID, groupPolicy: allowlist)"); + return denied; + } + const senderGroupAccess = evaluateSenderGroupAccessForPolicy({ + groupPolicy, + groupAllowFrom: effectiveGroupAllow.entries, + senderId, + isSenderAllowed: (candidateSenderId, allowFrom) => + isSenderAllowed({ + allow: normalizeAllowFrom(allowFrom), + senderId: candidateSenderId, + }), + }); + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "disabled") { logVerbose("Blocked line group message (groupPolicy: disabled)"); return denied; } - if (groupPolicy === "allowlist") { - if (!senderId) { - logVerbose("Blocked line group message (no sender ID, groupPolicy: allowlist)"); - return denied; - } - if (!effectiveGroupAllow.hasEntries) { - logVerbose("Blocked line group message (groupPolicy: allowlist, no groupAllowFrom)"); - return denied; - } - if (!isSenderAllowed({ allow: effectiveGroupAllow, senderId })) { - logVerbose(`Blocked line group message from ${senderId} (groupPolicy: allowlist)`); - return denied; - } + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "empty_allowlist") { + logVerbose("Blocked line group message (groupPolicy: allowlist, no groupAllowFrom)"); + return denied; + } + if (!senderGroupAccess.allowed && senderGroupAccess.reason === "sender_not_allowlisted") { + logVerbose(`Blocked line group message from ${senderId} (groupPolicy: allowlist)`); + return denied; } return { allowed: true, diff --git a/src/plugin-sdk/group-access.test.ts b/src/plugin-sdk/group-access.test.ts index 77eaf7a0fa2..135b7b92ff9 100644 --- a/src/plugin-sdk/group-access.test.ts +++ b/src/plugin-sdk/group-access.test.ts @@ -1,5 +1,33 @@ import { describe, expect, it } from "vitest"; -import { evaluateSenderGroupAccess } from "./group-access.js"; +import { evaluateSenderGroupAccess, evaluateSenderGroupAccessForPolicy } from "./group-access.js"; + +describe("evaluateSenderGroupAccessForPolicy", () => { + it("blocks disabled policy", () => { + const decision = evaluateSenderGroupAccessForPolicy({ + groupPolicy: "disabled", + groupAllowFrom: ["123"], + senderId: "123", + isSenderAllowed: () => true, + }); + + expect(decision).toMatchObject({ allowed: false, reason: "disabled", groupPolicy: "disabled" }); + }); + + it("blocks allowlist with empty list", () => { + const decision = evaluateSenderGroupAccessForPolicy({ + groupPolicy: "allowlist", + groupAllowFrom: [], + senderId: "123", + isSenderAllowed: () => true, + }); + + expect(decision).toMatchObject({ + allowed: false, + reason: "empty_allowlist", + groupPolicy: "allowlist", + }); + }); +}); describe("evaluateSenderGroupAccess", () => { it("defaults missing provider config to allowlist", () => { diff --git a/src/plugin-sdk/group-access.ts b/src/plugin-sdk/group-access.ts index 872b7dc8d76..d324c80489b 100644 --- a/src/plugin-sdk/group-access.ts +++ b/src/plugin-sdk/group-access.ts @@ -14,6 +14,48 @@ export type SenderGroupAccessDecision = { reason: SenderGroupAccessReason; }; +export function evaluateSenderGroupAccessForPolicy(params: { + groupPolicy: GroupPolicy; + providerMissingFallbackApplied?: boolean; + groupAllowFrom: string[]; + senderId: string; + isSenderAllowed: (senderId: string, allowFrom: string[]) => boolean; +}): SenderGroupAccessDecision { + if (params.groupPolicy === "disabled") { + return { + allowed: false, + groupPolicy: params.groupPolicy, + providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied), + reason: "disabled", + }; + } + if (params.groupPolicy === "allowlist") { + if (params.groupAllowFrom.length === 0) { + return { + allowed: false, + groupPolicy: params.groupPolicy, + providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied), + reason: "empty_allowlist", + }; + } + if (!params.isSenderAllowed(params.senderId, params.groupAllowFrom)) { + return { + allowed: false, + groupPolicy: params.groupPolicy, + providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied), + reason: "sender_not_allowlisted", + }; + } + } + + return { + allowed: true, + groupPolicy: params.groupPolicy, + providerMissingFallbackApplied: Boolean(params.providerMissingFallbackApplied), + reason: "allowed", + }; +} + export function evaluateSenderGroupAccess(params: { providerConfigPresent: boolean; configuredGroupPolicy?: GroupPolicy; @@ -28,37 +70,11 @@ export function evaluateSenderGroupAccess(params: { defaultGroupPolicy: params.defaultGroupPolicy, }); - if (groupPolicy === "disabled") { - return { - allowed: false, - groupPolicy, - providerMissingFallbackApplied, - reason: "disabled", - }; - } - if (groupPolicy === "allowlist") { - if (params.groupAllowFrom.length === 0) { - return { - allowed: false, - groupPolicy, - providerMissingFallbackApplied, - reason: "empty_allowlist", - }; - } - if (!params.isSenderAllowed(params.senderId, params.groupAllowFrom)) { - return { - allowed: false, - groupPolicy, - providerMissingFallbackApplied, - reason: "sender_not_allowlisted", - }; - } - } - - return { - allowed: true, + return evaluateSenderGroupAccessForPolicy({ groupPolicy, providerMissingFallbackApplied, - reason: "allowed", - }; + groupAllowFrom: params.groupAllowFrom, + senderId: params.senderId, + isSenderAllowed: params.isSenderAllowed, + }); } diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index 0a8b55bf23f..feb61839ef9 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -277,6 +277,7 @@ export { } from "./allow-from.js"; export { evaluateSenderGroupAccess, + evaluateSenderGroupAccessForPolicy, type SenderGroupAccessDecision, type SenderGroupAccessReason, } from "./group-access.js"; diff --git a/src/plugin-sdk/mattermost.ts b/src/plugin-sdk/mattermost.ts index ddcea319082..c680b6606c8 100644 --- a/src/plugin-sdk/mattermost.ts +++ b/src/plugin-sdk/mattermost.ts @@ -95,6 +95,7 @@ export { resolveDmGroupAccessWithLists, resolveEffectiveAllowFromLists, } from "../security/dm-policy-shared.js"; +export { evaluateSenderGroupAccessForPolicy } from "./group-access.js"; export type { WizardPrompter } from "../wizard/prompts.js"; export { buildAgentMediaPayload } from "./agent-media-payload.js"; export { loadOutboundMediaFromUrl } from "./outbound-media.js"; diff --git a/src/plugin-sdk/msteams.ts b/src/plugin-sdk/msteams.ts index 8b68b52b105..65b6fd21433 100644 --- a/src/plugin-sdk/msteams.ts +++ b/src/plugin-sdk/msteams.ts @@ -91,6 +91,7 @@ export { resolveDmGroupAccessWithLists, resolveEffectiveAllowFromLists, } from "../security/dm-policy-shared.js"; +export { evaluateSenderGroupAccessForPolicy } from "./group-access.js"; export { formatDocsLink } from "../terminal/links.js"; export { sleep } from "../utils.js"; export { loadWebMedia } from "../web/media.js";