From 892a9c24b0f6118729ab5b5f5499b1a7e792dd15 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 13:06:27 +0100 Subject: [PATCH] refactor(security): centralize channel allowlist auth policy --- extensions/irc/src/inbound.policy.test.ts | 3 + extensions/irc/src/inbound.ts | 14 +++- .../mattermost/src/mattermost/monitor-auth.ts | 58 ++++++++++++++ .../src/mattermost/monitor.authz.test.ts | 2 +- .../mattermost/src/mattermost/monitor.ts | 80 +++---------------- .../src/monitor-handler/message-handler.ts | 17 ++-- src/channels/allow-from.test.ts | 10 +++ src/channels/allow-from.ts | 10 ++- src/imessage/monitor/inbound-processing.ts | 17 ++-- src/security/dm-policy-shared.test.ts | 11 +++ src/security/dm-policy-shared.ts | 4 + .../bot-message-context.test-harness.ts | 1 + 12 files changed, 137 insertions(+), 90 deletions(-) create mode 100644 extensions/mattermost/src/mattermost/monitor-auth.ts diff --git a/extensions/irc/src/inbound.policy.test.ts b/extensions/irc/src/inbound.policy.test.ts index c5b6cdfac89..c3c317c5000 100644 --- a/extensions/irc/src/inbound.policy.test.ts +++ b/extensions/irc/src/inbound.policy.test.ts @@ -7,6 +7,7 @@ describe("irc inbound policy", () => { configAllowFrom: ["owner"], configGroupAllowFrom: [], storeAllowList: ["paired-user"], + dmPolicy: "pairing", }); expect(resolved.effectiveAllowFrom).toEqual(["owner", "paired-user"]); @@ -17,6 +18,7 @@ describe("irc inbound policy", () => { configAllowFrom: ["owner"], configGroupAllowFrom: ["group-owner"], storeAllowList: ["paired-user"], + dmPolicy: "pairing", }); expect(resolved.effectiveGroupAllowFrom).toEqual(["group-owner"]); @@ -27,6 +29,7 @@ describe("irc inbound policy", () => { configAllowFrom: ["owner"], configGroupAllowFrom: [], storeAllowList: ["paired-user"], + dmPolicy: "pairing", }); expect(resolved.effectiveGroupAllowFrom).toEqual([]); diff --git a/extensions/irc/src/inbound.ts b/extensions/irc/src/inbound.ts index efb0b781d4a..2efe735f228 100644 --- a/extensions/irc/src/inbound.ts +++ b/extensions/irc/src/inbound.ts @@ -9,6 +9,7 @@ import { resolveOutboundMediaUrls, resolveAllowlistProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, + resolveEffectiveAllowFromLists, warnMissingProviderGroupPolicyFallbackOnce, type OutboundReplyPayload, type OpenClawConfig, @@ -35,13 +36,19 @@ function resolveIrcEffectiveAllowlists(params: { configAllowFrom: string[]; configGroupAllowFrom: string[]; storeAllowList: string[]; + dmPolicy: string; }): { effectiveAllowFrom: string[]; effectiveGroupAllowFrom: string[]; } { - const effectiveAllowFrom = [...params.configAllowFrom, ...params.storeAllowList].filter(Boolean); - // Pairing-store entries are DM approvals and must not widen group sender authorization. - const effectiveGroupAllowFrom = [...params.configGroupAllowFrom].filter(Boolean); + const { effectiveAllowFrom, effectiveGroupAllowFrom } = resolveEffectiveAllowFromLists({ + allowFrom: params.configAllowFrom, + groupAllowFrom: params.configGroupAllowFrom, + storeAllowFrom: params.storeAllowList, + dmPolicy: params.dmPolicy, + // IRC intentionally requires explicit groupAllowFrom; do not fallback to allowFrom. + groupAllowFromFallbackToAllowFrom: false, + }); return { effectiveAllowFrom, effectiveGroupAllowFrom }; } @@ -141,6 +148,7 @@ export async function handleIrcInbound(params: { configAllowFrom, configGroupAllowFrom, storeAllowList, + dmPolicy, }); const allowTextCommands = core.channel.commands.shouldHandleTextCommands({ diff --git a/extensions/mattermost/src/mattermost/monitor-auth.ts b/extensions/mattermost/src/mattermost/monitor-auth.ts new file mode 100644 index 00000000000..71dcb137133 --- /dev/null +++ b/extensions/mattermost/src/mattermost/monitor-auth.ts @@ -0,0 +1,58 @@ +import { resolveAllowlistMatchSimple, resolveEffectiveAllowFromLists } from "openclaw/plugin-sdk"; + +export function normalizeMattermostAllowEntry(entry: string): string { + const trimmed = entry.trim(); + if (!trimmed) { + return ""; + } + if (trimmed === "*") { + return "*"; + } + return trimmed + .replace(/^(mattermost|user):/i, "") + .replace(/^@/, "") + .toLowerCase(); +} + +export function normalizeMattermostAllowList(entries: Array): string[] { + const normalized = entries + .map((entry) => normalizeMattermostAllowEntry(String(entry))) + .filter(Boolean); + return Array.from(new Set(normalized)); +} + +export function resolveMattermostEffectiveAllowFromLists(params: { + allowFrom?: Array | null; + groupAllowFrom?: Array | null; + storeAllowFrom?: Array | null; + dmPolicy?: string | null; +}): { + effectiveAllowFrom: string[]; + effectiveGroupAllowFrom: string[]; +} { + return resolveEffectiveAllowFromLists({ + allowFrom: normalizeMattermostAllowList(params.allowFrom ?? []), + groupAllowFrom: normalizeMattermostAllowList(params.groupAllowFrom ?? []), + storeAllowFrom: normalizeMattermostAllowList(params.storeAllowFrom ?? []), + dmPolicy: params.dmPolicy, + }); +} + +export function isMattermostSenderAllowed(params: { + senderId: string; + senderName?: string; + allowFrom: string[]; + allowNameMatching?: boolean; +}): boolean { + const allowFrom = params.allowFrom; + if (allowFrom.length === 0) { + return false; + } + const match = resolveAllowlistMatchSimple({ + allowFrom, + senderId: normalizeMattermostAllowEntry(params.senderId), + senderName: params.senderName ? normalizeMattermostAllowEntry(params.senderName) : undefined, + allowNameMatching: params.allowNameMatching, + }); + return match.allowed; +} diff --git a/extensions/mattermost/src/mattermost/monitor.authz.test.ts b/extensions/mattermost/src/mattermost/monitor.authz.test.ts index 707f3b28bff..5671090857f 100644 --- a/extensions/mattermost/src/mattermost/monitor.authz.test.ts +++ b/extensions/mattermost/src/mattermost/monitor.authz.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { resolveMattermostEffectiveAllowFromLists } from "./monitor.js"; +import { resolveMattermostEffectiveAllowFromLists } from "./monitor-auth.js"; describe("mattermost monitor authz", () => { it("keeps DM allowlist merged with pairing-store entries", () => { diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 906a370327e..1ed2ee74e35 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -18,7 +18,6 @@ import { isDangerousNameMatchingEnabled, resolveControlCommandGate, resolveDmGroupAccessWithLists, - resolveEffectiveAllowFromLists, resolveAllowlistProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, resolveChannelMediaMaxBytes, @@ -38,6 +37,11 @@ import { type MattermostPost, type MattermostUser, } from "./client.js"; +import { + isMattermostSenderAllowed, + normalizeMattermostAllowList, + resolveMattermostEffectiveAllowFromLists, +} from "./monitor-auth.js"; import { createDedupeCache, formatInboundFromLabel, @@ -132,68 +136,6 @@ function channelChatType(kind: ChatType): "direct" | "group" | "channel" { return "channel"; } -function normalizeAllowEntry(entry: string): string { - const trimmed = entry.trim(); - if (!trimmed) { - return ""; - } - if (trimmed === "*") { - return "*"; - } - return trimmed - .replace(/^(mattermost|user):/i, "") - .replace(/^@/, "") - .toLowerCase(); -} - -function normalizeAllowList(entries: Array): string[] { - const normalized = entries.map((entry) => normalizeAllowEntry(String(entry))).filter(Boolean); - return Array.from(new Set(normalized)); -} - -export function resolveMattermostEffectiveAllowFromLists(params: { - allowFrom?: Array | null; - groupAllowFrom?: Array | null; - storeAllowFrom?: Array | null; - dmPolicy?: string | null; -}): { - effectiveAllowFrom: string[]; - effectiveGroupAllowFrom: string[]; -} { - return resolveEffectiveAllowFromLists({ - allowFrom: normalizeAllowList(params.allowFrom ?? []), - groupAllowFrom: normalizeAllowList(params.groupAllowFrom ?? []), - storeAllowFrom: normalizeAllowList(params.storeAllowFrom ?? []), - dmPolicy: params.dmPolicy, - }); -} - -function isSenderAllowed(params: { - senderId: string; - senderName?: string; - allowFrom: string[]; - allowNameMatching?: boolean; -}): boolean { - const allowFrom = params.allowFrom; - if (allowFrom.length === 0) { - return false; - } - if (allowFrom.includes("*")) { - return true; - } - const normalizedSenderId = normalizeAllowEntry(params.senderId); - const normalizedSenderName = params.senderName ? normalizeAllowEntry(params.senderName) : ""; - return allowFrom.some((entry) => { - if (entry === normalizedSenderId) { - return true; - } - if (params.allowNameMatching !== true) { - return false; - } - return normalizedSenderName ? entry === normalizedSenderName : false; - }); -} - type MattermostMediaInfo = { path: string; contentType?: string; @@ -418,7 +360,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} senderId; const rawText = post.message?.trim() || ""; const dmPolicy = account.config.dmPolicy ?? "pairing"; - const storeAllowFrom = normalizeAllowList( + const storeAllowFrom = normalizeMattermostAllowList( dmPolicy === "allowlist" ? [] : await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []), @@ -437,13 +379,13 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} const hasControlCommand = core.channel.text.hasControlCommand(rawText, cfg); const isControlCommand = allowTextCommands && hasControlCommand; const useAccessGroups = cfg.commands?.useAccessGroups !== false; - const senderAllowedForCommands = isSenderAllowed({ + const senderAllowedForCommands = isMattermostSenderAllowed({ senderId, senderName, allowFrom: effectiveAllowFrom, allowNameMatching, }); - const groupAllowedForCommands = isSenderAllowed({ + const groupAllowedForCommands = isMattermostSenderAllowed({ senderId, senderName, allowFrom: effectiveGroupAllowFrom, @@ -901,7 +843,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} // Enforce DM/group policy and allowlist checks (same as normal messages) const dmPolicy = account.config.dmPolicy ?? "pairing"; - const storeAllowFrom = normalizeAllowList( + const storeAllowFrom = normalizeMattermostAllowList( dmPolicy === "allowlist" ? [] : await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []), @@ -914,10 +856,10 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} groupAllowFrom: account.config.groupAllowFrom, storeAllowFrom, isSenderAllowed: (allowFrom) => - isSenderAllowed({ + isMattermostSenderAllowed({ senderId: userId, senderName, - allowFrom: normalizeAllowList(allowFrom), + allowFrom: normalizeMattermostAllowList(allowFrom), allowNameMatching, }), }); diff --git a/extensions/msteams/src/monitor-handler/message-handler.ts b/extensions/msteams/src/monitor-handler/message-handler.ts index a87f704a340..e208d138c3f 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.ts @@ -9,6 +9,7 @@ import { isDangerousNameMatchingEnabled, resolveMentionGating, formatAllowlistMatchMeta, + resolveEffectiveAllowFromLists, type HistoryEntry, } from "openclaw/plugin-sdk"; import { @@ -136,7 +137,14 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { // Check DM policy for direct messages. const dmAllowFrom = msteamsCfg?.allowFrom ?? []; const configuredDmAllowFrom = dmAllowFrom.map((v) => String(v)); - const effectiveDmAllowFrom = [...configuredDmAllowFrom, ...storedAllowFrom]; + const groupAllowFrom = msteamsCfg?.groupAllowFrom; + const resolvedAllowFromLists = resolveEffectiveAllowFromLists({ + allowFrom: configuredDmAllowFrom, + groupAllowFrom, + storeAllowFrom: storedAllowFrom, + dmPolicy, + }); + const effectiveDmAllowFrom = resolvedAllowFromLists.effectiveAllowFrom; if (isDirectMessage && msteamsCfg) { const allowFrom = dmAllowFrom; @@ -184,13 +192,8 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { !isDirectMessage && msteamsCfg ? (msteamsCfg.groupPolicy ?? defaultGroupPolicy ?? "allowlist") : "disabled"; - const groupAllowFrom = - !isDirectMessage && msteamsCfg - ? (msteamsCfg.groupAllowFrom ?? - (msteamsCfg.allowFrom && msteamsCfg.allowFrom.length > 0 ? msteamsCfg.allowFrom : [])) - : []; const effectiveGroupAllowFrom = - !isDirectMessage && msteamsCfg ? groupAllowFrom.map((v) => String(v)) : []; + !isDirectMessage && msteamsCfg ? resolvedAllowFromLists.effectiveGroupAllowFrom : []; const teamId = activity.channelData?.team?.id; const teamName = activity.channelData?.team?.name; const channelName = activity.channelData?.channel?.name; diff --git a/src/channels/allow-from.test.ts b/src/channels/allow-from.test.ts index 8ae7262dab1..35ae9b57639 100644 --- a/src/channels/allow-from.test.ts +++ b/src/channels/allow-from.test.ts @@ -55,6 +55,16 @@ describe("resolveGroupAllowFromSources", () => { }), ).toEqual(["owner", "owner2"]); }); + + it("can disable fallback to DM allowlist", () => { + expect( + resolveGroupAllowFromSources({ + allowFrom: ["owner", "owner2"], + groupAllowFrom: [], + fallbackToAllowFrom: false, + }), + ).toEqual([]); + }); }); describe("firstDefined", () => { diff --git a/src/channels/allow-from.ts b/src/channels/allow-from.ts index 6d62fdc22d0..3e7591f2347 100644 --- a/src/channels/allow-from.ts +++ b/src/channels/allow-from.ts @@ -12,10 +12,16 @@ export function mergeDmAllowFromSources(params: { export function resolveGroupAllowFromSources(params: { allowFrom?: Array; groupAllowFrom?: Array; + fallbackToAllowFrom?: boolean; }): string[] { - const scoped = - params.groupAllowFrom && params.groupAllowFrom.length > 0 + const explicitGroupAllowFrom = + Array.isArray(params.groupAllowFrom) && params.groupAllowFrom.length > 0 ? params.groupAllowFrom + : undefined; + const scoped = explicitGroupAllowFrom + ? explicitGroupAllowFrom + : params.fallbackToAllowFrom === false + ? [] : (params.allowFrom ?? []); return scoped.map((value) => String(value).trim()).filter(Boolean); } diff --git a/src/imessage/monitor/inbound-processing.ts b/src/imessage/monitor/inbound-processing.ts index cf51e958b31..cbfaf051f18 100644 --- a/src/imessage/monitor/inbound-processing.ts +++ b/src/imessage/monitor/inbound-processing.ts @@ -20,6 +20,7 @@ import { resolveChannelGroupRequireMention, } from "../../config/group-policy.js"; import { resolveAgentRoute } from "../../routing/resolve-route.js"; +import { resolveEffectiveAllowFromLists } from "../../security/dm-policy-shared.js"; import { truncateUtf16Safe } from "../../utils.js"; import { formatIMessageChatTarget, @@ -138,14 +139,14 @@ export function resolveIMessageInboundDecision(params: { } const groupId = isGroup ? groupIdCandidate : undefined; - const storeAllowFrom = params.dmPolicy === "allowlist" ? [] : params.storeAllowFrom; - const effectiveDmAllowFrom = Array.from(new Set([...params.allowFrom, ...storeAllowFrom])) - .map((v) => String(v).trim()) - .filter(Boolean); - // Keep DM pairing-store authorization scoped to DMs; group access must come from explicit group allowlist config. - const effectiveGroupAllowFrom = Array.from(new Set(params.groupAllowFrom)) - .map((v) => String(v).trim()) - .filter(Boolean); + const { effectiveAllowFrom: effectiveDmAllowFrom, effectiveGroupAllowFrom } = + resolveEffectiveAllowFromLists({ + allowFrom: params.allowFrom, + groupAllowFrom: params.groupAllowFrom, + storeAllowFrom: params.storeAllowFrom, + dmPolicy: params.dmPolicy, + groupAllowFromFallbackToAllowFrom: false, + }); if (isGroup) { if (params.groupPolicy === "disabled") { diff --git a/src/security/dm-policy-shared.test.ts b/src/security/dm-policy-shared.test.ts index f24c0ea31d4..f421fe3b9f3 100644 --- a/src/security/dm-policy-shared.test.ts +++ b/src/security/dm-policy-shared.test.ts @@ -54,6 +54,17 @@ describe("security/dm-policy-shared", () => { expect(lists.effectiveGroupAllowFrom).toEqual(["owner"]); }); + it("can keep group allowlist empty when fallback is disabled", () => { + const lists = resolveEffectiveAllowFromLists({ + allowFrom: ["owner"], + groupAllowFrom: [], + storeAllowFrom: ["paired-user"], + groupAllowFromFallbackToAllowFrom: false, + }); + expect(lists.effectiveAllowFrom).toEqual(["owner", "paired-user"]); + expect(lists.effectiveGroupAllowFrom).toEqual([]); + }); + it("excludes storeAllowFrom when dmPolicy is allowlist", () => { const lists = resolveEffectiveAllowFromLists({ allowFrom: ["+1111"], diff --git a/src/security/dm-policy-shared.ts b/src/security/dm-policy-shared.ts index 2da4b936cca..89f7740ce05 100644 --- a/src/security/dm-policy-shared.ts +++ b/src/security/dm-policy-shared.ts @@ -8,6 +8,7 @@ export function resolveEffectiveAllowFromLists(params: { groupAllowFrom?: Array | null; storeAllowFrom?: Array | null; dmPolicy?: string | null; + groupAllowFromFallbackToAllowFrom?: boolean | null; }): { effectiveAllowFrom: string[]; effectiveGroupAllowFrom: string[]; @@ -27,6 +28,7 @@ export function resolveEffectiveAllowFromLists(params: { resolveGroupAllowFromSources({ allowFrom, groupAllowFrom, + fallbackToAllowFrom: params.groupAllowFromFallbackToAllowFrom ?? undefined, }), ); return { effectiveAllowFrom, effectiveGroupAllowFrom }; @@ -87,6 +89,7 @@ export function resolveDmGroupAccessWithLists(params: { allowFrom?: Array | null; groupAllowFrom?: Array | null; storeAllowFrom?: Array | null; + groupAllowFromFallbackToAllowFrom?: boolean | null; isSenderAllowed: (allowFrom: string[]) => boolean; }): { decision: DmGroupAccessDecision; @@ -99,6 +102,7 @@ export function resolveDmGroupAccessWithLists(params: { groupAllowFrom: params.groupAllowFrom, storeAllowFrom: params.storeAllowFrom, dmPolicy: params.dmPolicy, + groupAllowFromFallbackToAllowFrom: params.groupAllowFromFallbackToAllowFrom, }); const access = resolveDmGroupAccessDecision({ isGroup: params.isGroup, diff --git a/src/telegram/bot-message-context.test-harness.ts b/src/telegram/bot-message-context.test-harness.ts index afdbbffce68..acfb84e6d69 100644 --- a/src/telegram/bot-message-context.test-harness.ts +++ b/src/telegram/bot-message-context.test-harness.ts @@ -61,5 +61,6 @@ export async function buildTelegramMessageContextForTest( groupConfig: { requireMention: false }, topicConfig: undefined, })), + sendChatActionHandler: { sendChatAction: vi.fn() } as never, }); }