From 8f8e46d898c55a5b37d4b6d6f1b446e04f104a4e Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 01:34:37 +0100 Subject: [PATCH] refactor: unify reaction ingress policy guards across channels --- .../bluebubbles/src/monitor-processing.ts | 43 ++- .../mattermost/src/mattermost/monitor.ts | 83 ++---- src/discord/monitor/listeners.ts | 263 ++++++++++++------ src/plugin-sdk/index.ts | 1 + src/security/dm-policy-shared.test.ts | 64 +++++ src/signal/monitor/event-handler.ts | 23 +- 6 files changed, 289 insertions(+), 188 deletions(-) diff --git a/extensions/bluebubbles/src/monitor-processing.ts b/extensions/bluebubbles/src/monitor-processing.ts index 67fb50a78..f25e47d50 100644 --- a/extensions/bluebubbles/src/monitor-processing.ts +++ b/extensions/bluebubbles/src/monitor-processing.ts @@ -7,8 +7,7 @@ import { logTypingFailure, recordPendingHistoryEntryIfEnabled, resolveAckReaction, - resolveDmGroupAccessDecision, - resolveEffectiveAllowFromLists, + resolveDmGroupAccessWithLists, resolveControlCommandGate, stripMarkdown, type HistoryEntry, @@ -504,24 +503,13 @@ export async function processMessage( const storeAllowFrom = await core.channel.pairing .readAllowFromStore("bluebubbles") .catch(() => []); - const { effectiveAllowFrom, effectiveGroupAllowFrom } = resolveEffectiveAllowFromLists({ - allowFrom: account.config.allowFrom, - groupAllowFrom: account.config.groupAllowFrom, - storeAllowFrom, - dmPolicy, - }); - const groupAllowEntry = formatGroupAllowlistEntry({ - chatGuid: message.chatGuid, - chatId: message.chatId ?? undefined, - chatIdentifier: message.chatIdentifier ?? undefined, - }); - const groupName = message.chatName?.trim() || undefined; - const accessDecision = resolveDmGroupAccessDecision({ + const accessDecision = resolveDmGroupAccessWithLists({ isGroup, dmPolicy, groupPolicy, - effectiveAllowFrom, - effectiveGroupAllowFrom, + allowFrom: account.config.allowFrom, + groupAllowFrom: account.config.groupAllowFrom, + storeAllowFrom, isSenderAllowed: (allowFrom) => isAllowedBlueBubblesSender({ allowFrom, @@ -531,6 +519,14 @@ export async function processMessage( chatIdentifier: message.chatIdentifier ?? undefined, }), }); + const effectiveAllowFrom = accessDecision.effectiveAllowFrom; + const effectiveGroupAllowFrom = accessDecision.effectiveGroupAllowFrom; + const groupAllowEntry = formatGroupAllowlistEntry({ + chatGuid: message.chatGuid, + chatId: message.chatId ?? undefined, + chatIdentifier: message.chatIdentifier ?? undefined, + }); + const groupName = message.chatName?.trim() || undefined; if (accessDecision.decision !== "allow") { if (isGroup) { @@ -1389,18 +1385,13 @@ export async function processReaction( const storeAllowFrom = await core.channel.pairing .readAllowFromStore("bluebubbles") .catch(() => []); - const { effectiveAllowFrom, effectiveGroupAllowFrom } = resolveEffectiveAllowFromLists({ - allowFrom: account.config.allowFrom, - groupAllowFrom: account.config.groupAllowFrom, - storeAllowFrom, - dmPolicy, - }); - const accessDecision = resolveDmGroupAccessDecision({ + const accessDecision = resolveDmGroupAccessWithLists({ isGroup: reaction.isGroup, dmPolicy, groupPolicy, - effectiveAllowFrom, - effectiveGroupAllowFrom, + allowFrom: account.config.allowFrom, + groupAllowFrom: account.config.groupAllowFrom, + storeAllowFrom, isSenderAllowed: (allowFrom) => isAllowedBlueBubblesSender({ allowFrom, diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 6056c3fef..af8d8e07e 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -17,6 +17,7 @@ import { recordPendingHistoryEntryIfEnabled, isDangerousNameMatchingEnabled, resolveControlCommandGate, + resolveDmGroupAccessWithLists, resolveAllowlistProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, resolveChannelMediaMaxBytes, @@ -883,68 +884,38 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} const kind = channelKind(channelInfo.type); // Enforce DM/group policy and allowlist checks (same as normal messages) - if (kind === "direct") { - const dmPolicy = account.config.dmPolicy ?? "pairing"; - if (dmPolicy === "disabled") { - logVerboseMessage(`mattermost: drop reaction (dmPolicy=disabled sender=${userId})`); - return; - } - // For pairing/allowlist modes, only allow reactions from approved senders - if (dmPolicy !== "open") { - const configAllowFrom = normalizeAllowList(account.config.allowFrom ?? []); - const storeAllowFrom = normalizeAllowList( - dmPolicy === "allowlist" - ? [] - : await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []), - ); - const effectiveAllowFrom = Array.from(new Set([...configAllowFrom, ...storeAllowFrom])); - const allowed = isSenderAllowed({ + const dmPolicy = account.config.dmPolicy ?? "pairing"; + const storeAllowFrom = normalizeAllowList( + dmPolicy === "allowlist" + ? [] + : await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []), + ); + const reactionAccess = resolveDmGroupAccessWithLists({ + isGroup: kind !== "direct", + dmPolicy, + groupPolicy, + allowFrom: account.config.allowFrom, + groupAllowFrom: account.config.groupAllowFrom, + storeAllowFrom, + isSenderAllowed: (allowFrom) => + isSenderAllowed({ senderId: userId, senderName, - allowFrom: effectiveAllowFrom, + allowFrom: normalizeAllowList(allowFrom), allowNameMatching, - }); - if (!allowed) { - logVerboseMessage( - `mattermost: drop reaction (dmPolicy=${dmPolicy} sender=${userId} not allowed)`, - ); - return; - } - } - } else if (kind) { - if (groupPolicy === "disabled") { - logVerboseMessage(`mattermost: drop reaction (groupPolicy=disabled channel=${channelId})`); - return; - } - if (groupPolicy === "allowlist") { - const dmPolicyForStore = account.config.dmPolicy ?? "pairing"; - const configAllowFrom = normalizeAllowList(account.config.allowFrom ?? []); - const configGroupAllowFrom = normalizeAllowList(account.config.groupAllowFrom ?? []); - const storeAllowFrom = normalizeAllowList( - dmPolicyForStore === "allowlist" - ? [] - : await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []), + }), + }); + if (reactionAccess.decision !== "allow") { + if (kind === "direct") { + logVerboseMessage( + `mattermost: drop reaction (dmPolicy=${dmPolicy} sender=${userId} reason=${reactionAccess.reason})`, ); - const effectiveGroupAllowFrom = Array.from( - new Set([ - ...(configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom), - ...storeAllowFrom, - ]), + } else { + logVerboseMessage( + `mattermost: drop reaction (groupPolicy=${groupPolicy} sender=${userId} reason=${reactionAccess.reason} channel=${channelId})`, ); - // Drop when allowlist is empty (same as normal message handler) - const allowed = - effectiveGroupAllowFrom.length > 0 && - isSenderAllowed({ - senderId: userId, - senderName, - allowFrom: effectiveGroupAllowFrom, - allowNameMatching, - }); - if (!allowed) { - logVerboseMessage(`mattermost: drop reaction (groupPolicy=allowlist sender=${userId})`); - return; - } } + return; } const teamId = channelInfo?.team_id ?? undefined; diff --git a/src/discord/monitor/listeners.ts b/src/discord/monitor/listeners.ts index 002bf6281..c8af895ad 100644 --- a/src/discord/monitor/listeners.ts +++ b/src/discord/monitor/listeners.ts @@ -204,6 +204,99 @@ async function runDiscordReactionHandler(params: { }); } +type DiscordReactionIngressAuthorizationParams = { + user: User; + isDirectMessage: boolean; + isGroupDm: boolean; + isGuildMessage: boolean; + channelId: string; + channelName?: string; + channelSlug: string; + dmEnabled: boolean; + groupDmEnabled: boolean; + groupDmChannels: string[]; + dmPolicy: "open" | "pairing" | "allowlist" | "disabled"; + allowFrom: string[]; + groupPolicy: "open" | "allowlist" | "disabled"; + allowNameMatching: boolean; + guildInfo: import("./allow-list.js").DiscordGuildEntryResolved | null; + channelConfig?: { allowed?: boolean } | null; +}; + +async function authorizeDiscordReactionIngress( + params: DiscordReactionIngressAuthorizationParams, +): Promise<{ allowed: true } | { allowed: false; reason: string }> { + if (params.isDirectMessage && !params.dmEnabled) { + return { allowed: false, reason: "dm-disabled" }; + } + if (params.isGroupDm && !params.groupDmEnabled) { + return { allowed: false, reason: "group-dm-disabled" }; + } + if (params.isDirectMessage) { + const storeAllowFrom = + params.dmPolicy === "allowlist" + ? [] + : await readChannelAllowFromStore("discord").catch(() => []); + const access = resolveDmGroupAccessWithLists({ + isGroup: false, + dmPolicy: params.dmPolicy, + groupPolicy: params.groupPolicy, + allowFrom: params.allowFrom, + groupAllowFrom: [], + storeAllowFrom, + isSenderAllowed: (allowEntries) => { + const allowList = normalizeDiscordAllowList(allowEntries, ["discord:", "user:", "pk:"]); + const allowMatch = allowList + ? resolveDiscordAllowListMatch({ + allowList, + candidate: { + id: params.user.id, + name: params.user.username, + tag: formatDiscordUserTag(params.user), + }, + allowNameMatching: params.allowNameMatching, + }) + : { allowed: false }; + return allowMatch.allowed; + }, + }); + if (access.decision !== "allow") { + return { allowed: false, reason: access.reason }; + } + } + if ( + params.isGroupDm && + !resolveGroupDmAllow({ + channels: params.groupDmChannels, + channelId: params.channelId, + channelName: params.channelName, + channelSlug: params.channelSlug, + }) + ) { + return { allowed: false, reason: "group-dm-not-allowlisted" }; + } + if (!params.isGuildMessage) { + return { allowed: true }; + } + const channelAllowlistConfigured = + Boolean(params.guildInfo?.channels) && Object.keys(params.guildInfo?.channels ?? {}).length > 0; + const channelAllowed = params.channelConfig?.allowed !== false; + if ( + !isDiscordGroupAllowedByPolicy({ + groupPolicy: params.groupPolicy, + guildAllowlisted: Boolean(params.guildInfo), + channelAllowlistConfigured, + channelAllowed, + }) + ) { + return { allowed: false, reason: "guild-policy" }; + } + if (params.channelConfig?.allowed === false) { + return { allowed: false, reason: "guild-channel-denied" }; + } + return { allowed: true }; +} + async function handleDiscordReactionEvent(params: { data: DiscordReactionEvent; client: Client; @@ -260,10 +353,25 @@ async function handleDiscordReactionEvent(params: { channelType === ChannelType.PublicThread || channelType === ChannelType.PrivateThread || channelType === ChannelType.AnnouncementThread; - if (isDirectMessage && !params.dmEnabled) { - return; - } - if (isGroupDm && !params.groupDmEnabled) { + const ingressAccess = await authorizeDiscordReactionIngress({ + user, + isDirectMessage, + isGroupDm, + isGuildMessage, + channelId: data.channel_id, + channelName, + channelSlug, + dmEnabled: params.dmEnabled, + groupDmEnabled: params.groupDmEnabled, + groupDmChannels: params.groupDmChannels, + dmPolicy: params.dmPolicy, + allowFrom: params.allowFrom, + groupPolicy: params.groupPolicy, + allowNameMatching: params.allowNameMatching, + guildInfo, + }); + if (!ingressAccess.allowed) { + logVerbose(`discord reaction blocked sender=${user.id} (reason=${ingressAccess.reason})`); return; } let parentId = "parentId" in channel ? (channel.parentId ?? undefined) : undefined; @@ -294,45 +402,6 @@ async function handleDiscordReactionEvent(params: { reactionBase = { baseText, contextKey }; return reactionBase; }; - const isDirectReactionAuthorized = async () => { - if (!isDirectMessage) { - return true; - } - const storeAllowFrom = - params.dmPolicy === "allowlist" - ? [] - : await readChannelAllowFromStore("discord").catch(() => []); - const access = resolveDmGroupAccessWithLists({ - isGroup: false, - dmPolicy: params.dmPolicy, - groupPolicy: params.groupPolicy, - allowFrom: params.allowFrom, - groupAllowFrom: [], - storeAllowFrom, - isSenderAllowed: (allowEntries) => { - const allowList = normalizeDiscordAllowList(allowEntries, ["discord:", "user:", "pk:"]); - const allowMatch = allowList - ? resolveDiscordAllowListMatch({ - allowList, - candidate: { - id: user.id, - name: user.username, - tag: formatDiscordUserTag(user), - }, - allowNameMatching: params.allowNameMatching, - }) - : { allowed: false }; - return allowMatch.allowed; - }, - }); - if (access.decision !== "allow") { - logVerbose( - `discord reaction blocked sender=${user.id} (dmPolicy=${params.dmPolicy}, decision=${access.decision}, reason=${access.reason})`, - ); - return false; - } - return true; - }; const emitReaction = (text: string, parentPeerId?: string) => { const { contextKey } = resolveReactionBase(); const route = resolveAgentRoute({ @@ -391,44 +460,6 @@ async function handleDiscordReactionEvent(params: { parentSlug, scope: "thread", }); - const isGuildReactionAllowed = (channelConfig: { allowed?: boolean } | null) => { - if (!isGuildMessage) { - return true; - } - const channelAllowlistConfigured = - Boolean(guildInfo?.channels) && Object.keys(guildInfo?.channels ?? {}).length > 0; - const channelAllowed = channelConfig?.allowed !== false; - if ( - !isDiscordGroupAllowedByPolicy({ - groupPolicy: params.groupPolicy, - guildAllowlisted: Boolean(guildInfo), - channelAllowlistConfigured, - channelAllowed, - }) - ) { - return false; - } - if (channelConfig?.allowed === false) { - return false; - } - return true; - }; - - if (!(await isDirectReactionAuthorized())) { - return; - } - - if ( - isGroupDm && - !resolveGroupDmAllow({ - channels: params.groupDmChannels, - channelId: data.channel_id, - channelName, - channelSlug, - }) - ) { - return; - } // Parallelize async operations for thread channels if (isThreadChannel) { @@ -450,7 +481,25 @@ async function handleDiscordReactionEvent(params: { await loadThreadParentInfo(); const channelConfig = resolveThreadChannelConfig(); - if (channelConfig?.allowed === false) { + const threadAccess = await authorizeDiscordReactionIngress({ + user, + isDirectMessage, + isGroupDm, + isGuildMessage, + channelId: data.channel_id, + channelName, + channelSlug, + dmEnabled: params.dmEnabled, + groupDmEnabled: params.groupDmEnabled, + groupDmChannels: params.groupDmChannels, + dmPolicy: params.dmPolicy, + allowFrom: params.allowFrom, + groupPolicy: params.groupPolicy, + allowNameMatching: params.allowNameMatching, + guildInfo, + channelConfig, + }); + if (!threadAccess.allowed) { return; } @@ -474,10 +523,25 @@ async function handleDiscordReactionEvent(params: { await loadThreadParentInfo(); const channelConfig = resolveThreadChannelConfig(); - if (channelConfig?.allowed === false) { - return; - } - if (!isGuildReactionAllowed(channelConfig)) { + const threadAccess = await authorizeDiscordReactionIngress({ + user, + isDirectMessage, + isGroupDm, + isGuildMessage, + channelId: data.channel_id, + channelName, + channelSlug, + dmEnabled: params.dmEnabled, + groupDmEnabled: params.groupDmEnabled, + groupDmChannels: params.groupDmChannels, + dmPolicy: params.dmPolicy, + allowFrom: params.allowFrom, + groupPolicy: params.groupPolicy, + allowNameMatching: params.allowNameMatching, + guildInfo, + channelConfig, + }); + if (!threadAccess.allowed) { return; } @@ -501,11 +565,28 @@ async function handleDiscordReactionEvent(params: { parentSlug, scope: "channel", }); - if (channelConfig?.allowed === false) { - return; - } - if (!isGuildReactionAllowed(channelConfig)) { - return; + if (isGuildMessage) { + const channelAccess = await authorizeDiscordReactionIngress({ + user, + isDirectMessage, + isGroupDm, + isGuildMessage, + channelId: data.channel_id, + channelName, + channelSlug, + dmEnabled: params.dmEnabled, + groupDmEnabled: params.groupDmEnabled, + groupDmChannels: params.groupDmChannels, + dmPolicy: params.dmPolicy, + allowFrom: params.allowFrom, + groupPolicy: params.groupPolicy, + allowNameMatching: params.allowNameMatching, + guildInfo, + channelConfig, + }); + if (!channelAccess.allowed) { + return; + } } const reactionMode = guildInfo?.reactionNotifications ?? "own"; diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index 7faa2341d..6e25d5074 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -375,6 +375,7 @@ export { formatDocsLink } from "../terminal/links.js"; export { resolveDmAllowState, resolveDmGroupAccessDecision, + resolveDmGroupAccessWithLists, resolveEffectiveAllowFromLists, } from "../security/dm-policy-shared.js"; export type { HookEntry } from "../hooks/types.js"; diff --git a/src/security/dm-policy-shared.test.ts b/src/security/dm-policy-shared.test.ts index 1fe36976a..735b7d872 100644 --- a/src/security/dm-policy-shared.test.ts +++ b/src/security/dm-policy-shared.test.ts @@ -118,6 +118,70 @@ describe("security/dm-policy-shared", () => { "zalo", ] as const; + it("keeps message/reaction policy parity table across channels", () => { + const cases = [ + { + name: "dmPolicy=open", + dmPolicy: "open" as const, + allowFrom: [] as string[], + senderAllowed: false, + expectedDecision: "allow" as const, + expectedReactionAllowed: true, + }, + { + name: "dmPolicy=disabled", + dmPolicy: "disabled" as const, + allowFrom: [] as string[], + senderAllowed: false, + expectedDecision: "block" as const, + expectedReactionAllowed: false, + }, + { + name: "dmPolicy=allowlist unauthorized", + dmPolicy: "allowlist" as const, + allowFrom: ["owner"], + senderAllowed: false, + expectedDecision: "block" as const, + expectedReactionAllowed: false, + }, + { + name: "dmPolicy=allowlist authorized", + dmPolicy: "allowlist" as const, + allowFrom: ["owner"], + senderAllowed: true, + expectedDecision: "allow" as const, + expectedReactionAllowed: true, + }, + { + name: "dmPolicy=pairing unauthorized", + dmPolicy: "pairing" as const, + allowFrom: [] as string[], + senderAllowed: false, + expectedDecision: "pairing" as const, + expectedReactionAllowed: false, + }, + ]; + + for (const channel of channels) { + for (const testCase of cases) { + const access = resolveDmGroupAccessWithLists({ + isGroup: false, + dmPolicy: testCase.dmPolicy, + groupPolicy: "allowlist", + allowFrom: testCase.allowFrom, + groupAllowFrom: [], + storeAllowFrom: [], + isSenderAllowed: () => testCase.senderAllowed, + }); + const reactionAllowed = access.decision === "allow"; + expect(access.decision, `[${channel}] ${testCase.name}`).toBe(testCase.expectedDecision); + expect(reactionAllowed, `[${channel}] ${testCase.name} reaction`).toBe( + testCase.expectedReactionAllowed, + ); + } + } + }); + for (const channel of channels) { it(`[${channel}] blocks DM allowlist mode when allowlist is empty`, () => { const decision = resolveDmGroupAccessDecision({ diff --git a/src/signal/monitor/event-handler.ts b/src/signal/monitor/event-handler.ts index e87158d8d..ea9fa9f49 100644 --- a/src/signal/monitor/event-handler.ts +++ b/src/signal/monitor/event-handler.ts @@ -36,10 +36,7 @@ import { upsertChannelPairingRequest, } from "../../pairing/pairing-store.js"; import { resolveAgentRoute } from "../../routing/resolve-route.js"; -import { - resolveDmGroupAccessDecision, - resolveEffectiveAllowFromLists, -} from "../../security/dm-policy-shared.js"; +import { resolveDmGroupAccessWithLists } from "../../security/dm-policy-shared.js"; import { normalizeE164 } from "../../utils.js"; import { formatSignalPairingIdLine, @@ -460,23 +457,19 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { deps.dmPolicy === "allowlist" ? [] : await readChannelAllowFromStore("signal").catch(() => []); - const { effectiveAllowFrom: effectiveDmAllow, effectiveGroupAllowFrom: effectiveGroupAllow } = - resolveEffectiveAllowFromLists({ - allowFrom: deps.allowFrom, - groupAllowFrom: deps.groupAllowFrom, - storeAllowFrom, - dmPolicy: deps.dmPolicy, - }); const resolveAccessDecision = (isGroup: boolean) => - resolveDmGroupAccessDecision({ + resolveDmGroupAccessWithLists({ isGroup, dmPolicy: deps.dmPolicy, groupPolicy: deps.groupPolicy, - effectiveAllowFrom: effectiveDmAllow, - effectiveGroupAllowFrom: effectiveGroupAllow, - isSenderAllowed: (allowFrom) => isSignalSenderAllowed(sender, allowFrom), + allowFrom: deps.allowFrom, + groupAllowFrom: deps.groupAllowFrom, + storeAllowFrom, + isSenderAllowed: (allowEntries) => isSignalSenderAllowed(sender, allowEntries), }); const dmAccess = resolveAccessDecision(false); + const effectiveDmAllow = dmAccess.effectiveAllowFrom; + const effectiveGroupAllow = dmAccess.effectiveGroupAllowFrom; const dmAllowed = dmAccess.decision === "allow"; if (