From 487a3ba8ceeffa0a5a5ba12d6d00d9d347b3d0d4 Mon Sep 17 00:00:00 2001 From: Robin Waslander Date: Thu, 12 Mar 2026 02:48:17 +0100 Subject: [PATCH] fix(discord): enforce users/roles allowlist in reaction ingress References GHSA-9vvh-2768-c8vp. --- src/discord/monitor.test.ts | 93 +++++++++++++++++++++++++++---- src/discord/monitor/allow-list.ts | 38 ++++++++----- src/discord/monitor/listeners.ts | 84 +++++++++++++++++++--------- 3 files changed, 164 insertions(+), 51 deletions(-) diff --git a/src/discord/monitor.test.ts b/src/discord/monitor.test.ts index 10c7dc66747..9471a3fe6bc 100644 --- a/src/discord/monitor.test.ts +++ b/src/discord/monitor.test.ts @@ -38,6 +38,7 @@ const makeEntries = ( requireMention: value.requireMention, reactionNotifications: value.reactionNotifications, users: value.users, + roles: value.roles, channels: value.channels, }; } @@ -730,6 +731,17 @@ describe("discord reaction notification gating", () => { }, expected: true, }, + { + name: "all mode blocks non-allowlisted guild member", + input: { + mode: "all" as const, + botId: "bot-1", + messageAuthorId: "user-1", + userId: "user-2", + guildInfo: { users: ["trusted-user"] }, + }, + expected: false, + }, { name: "own mode with bot-authored message", input: { @@ -750,6 +762,17 @@ describe("discord reaction notification gating", () => { }, expected: false, }, + { + name: "own mode still blocks member outside users allowlist", + input: { + mode: "own" as const, + botId: "bot-1", + messageAuthorId: "bot-1", + userId: "user-3", + guildInfo: { users: ["trusted-user"] }, + }, + expected: false, + }, { name: "allowlist mode without match", input: { @@ -769,7 +792,7 @@ describe("discord reaction notification gating", () => { messageAuthorId: "user-1", userId: "123", userName: "steipete", - allowlist: ["123", "other"] as string[], + guildInfo: { users: ["123", "other"] }, }, expected: true, }, @@ -781,7 +804,7 @@ describe("discord reaction notification gating", () => { messageAuthorId: "user-1", userId: "999", userName: "trusted-user", - allowlist: ["trusted-user"] as string[], + guildInfo: { users: ["trusted-user"] }, }, expected: false, }, @@ -793,21 +816,29 @@ describe("discord reaction notification gating", () => { messageAuthorId: "user-1", userId: "999", userName: "trusted-user", - allowlist: ["trusted-user"] as string[], + guildInfo: { users: ["trusted-user"] }, allowNameMatching: true, }, expected: true, }, + { + name: "allowlist mode matches allowed role", + input: { + mode: "allowlist" as const, + botId: "bot-1", + messageAuthorId: "user-1", + userId: "999", + guildInfo: { roles: ["role:trusted-role"] }, + memberRoleIds: ["trusted-role"], + }, + expected: true, + }, ]); for (const testCase of cases) { expect( shouldEmitDiscordReactionNotification({ ...testCase.input, - allowlist: - "allowlist" in testCase.input && testCase.input.allowlist - ? [...testCase.input.allowlist] - : undefined, }), testCase.name, ).toBe(testCase.expected); @@ -863,6 +894,7 @@ function makeReactionEvent(overrides?: { messageAuthorId?: string; messageFetch?: ReturnType; guild?: { name?: string; id?: string }; + memberRoleIds?: string[]; }) { const userId = overrides?.userId ?? "user-1"; const messageId = overrides?.messageId ?? "msg-1"; @@ -882,6 +914,7 @@ function makeReactionEvent(overrides?: { message_id: messageId, emoji: { name: overrides?.emojiName ?? "👍", id: null }, guild: overrides?.guild, + rawMember: overrides?.memberRoleIds ? { roles: overrides.memberRoleIds } : undefined, user: { id: userId, bot: false, @@ -1059,7 +1092,31 @@ describe("discord DM reaction handling", () => { expect(enqueueSystemEventSpy).not.toHaveBeenCalled(); }); - it("still processes guild reactions (no regression)", async () => { + it("blocks guild reactions for sender outside users allowlist", async () => { + const data = makeReactionEvent({ + guildId: "guild-123", + userId: "attacker-user", + botAsAuthor: true, + guild: { id: "guild-123", name: "Test Guild" }, + }); + const client = makeReactionClient({ channelType: ChannelType.GuildText }); + const listener = new DiscordReactionListener( + makeReactionListenerParams({ + guildEntries: makeEntries({ + "guild-123": { + users: ["user:trusted-user"], + }, + }), + }), + ); + + await listener.handle(data, client); + + expect(enqueueSystemEventSpy).not.toHaveBeenCalled(); + expect(resolveAgentRouteMock).not.toHaveBeenCalled(); + }); + + it("allows guild reactions for sender in channel role allowlist override", async () => { resolveAgentRouteMock.mockReturnValueOnce({ agentId: "default", channel: "discord", @@ -1069,11 +1126,27 @@ describe("discord DM reaction handling", () => { const data = makeReactionEvent({ guildId: "guild-123", + userId: "member-user", botAsAuthor: true, - guild: { name: "Test Guild" }, + guild: { id: "guild-123", name: "Test Guild" }, + memberRoleIds: ["trusted-role"], }); const client = makeReactionClient({ channelType: ChannelType.GuildText }); - const listener = new DiscordReactionListener(makeReactionListenerParams()); + const listener = new DiscordReactionListener( + makeReactionListenerParams({ + guildEntries: makeEntries({ + "guild-123": { + roles: ["role:blocked-role"], + channels: { + "channel-1": { + allow: true, + roles: ["role:trusted-role"], + }, + }, + }, + }), + }), + ); await listener.handle(data, client); diff --git a/src/discord/monitor/allow-list.ts b/src/discord/monitor/allow-list.ts index b736928e276..7c1250cb8ef 100644 --- a/src/discord/monitor/allow-list.ts +++ b/src/discord/monitor/allow-list.ts @@ -556,6 +556,9 @@ export function shouldEmitDiscordReactionNotification(params: { userId: string; userName?: string; userTag?: string; + channelConfig?: DiscordChannelConfigResolved | null; + guildInfo?: DiscordGuildEntryResolved | null; + memberRoleIds?: string[]; allowlist?: string[]; allowNameMatching?: boolean; }) { @@ -563,26 +566,31 @@ export function shouldEmitDiscordReactionNotification(params: { if (mode === "off") { return false; } + const accessGuildInfo = + params.guildInfo ?? + (params.allowlist ? ({ users: params.allowlist } satisfies DiscordGuildEntryResolved) : null); + const { hasAccessRestrictions, memberAllowed } = resolveDiscordMemberAccessState({ + channelConfig: params.channelConfig, + guildInfo: accessGuildInfo, + memberRoleIds: params.memberRoleIds ?? [], + sender: { + id: params.userId, + name: params.userName, + tag: params.userTag, + }, + allowNameMatching: params.allowNameMatching, + }); + if (mode === "allowlist") { + return hasAccessRestrictions && memberAllowed; + } + if (hasAccessRestrictions && !memberAllowed) { + return false; + } if (mode === "all") { return true; } if (mode === "own") { return Boolean(params.botId && params.messageAuthorId === params.botId); } - if (mode === "allowlist") { - const list = normalizeDiscordAllowList(params.allowlist, ["discord:", "user:", "pk:"]); - if (!list) { - return false; - } - return allowListMatches( - list, - { - id: params.userId, - name: params.userName, - tag: params.userTag, - }, - { allowNameMatching: params.allowNameMatching }, - ); - } return false; } diff --git a/src/discord/monitor/listeners.ts b/src/discord/monitor/listeners.ts index 056a1ad7116..824cb5fb19a 100644 --- a/src/discord/monitor/listeners.ts +++ b/src/discord/monitor/listeners.ts @@ -24,6 +24,7 @@ import { normalizeDiscordSlug, resolveDiscordAllowListMatch, resolveDiscordChannelConfigWithFallback, + resolveDiscordMemberAccessState, resolveGroupDmAllow, resolveDiscordGuildEntry, shouldEmitDiscordReactionNotification, @@ -294,6 +295,7 @@ async function runDiscordReactionHandler(params: { type DiscordReactionIngressAuthorizationParams = { accountId: string; user: User; + memberRoleIds: string[]; isDirectMessage: boolean; isGroupDm: boolean; isGuildMessage: boolean; @@ -308,7 +310,7 @@ type DiscordReactionIngressAuthorizationParams = { groupPolicy: "open" | "allowlist" | "disabled"; allowNameMatching: boolean; guildInfo: import("./allow-list.js").DiscordGuildEntryResolved | null; - channelConfig?: { allowed?: boolean } | null; + channelConfig?: import("./allow-list.js").DiscordChannelConfigResolved | null; }; async function authorizeDiscordReactionIngress( @@ -383,6 +385,20 @@ async function authorizeDiscordReactionIngress( if (params.channelConfig?.allowed === false) { return { allowed: false, reason: "guild-channel-denied" }; } + const { hasAccessRestrictions, memberAllowed } = resolveDiscordMemberAccessState({ + channelConfig: params.channelConfig, + guildInfo: params.guildInfo, + memberRoleIds: params.memberRoleIds, + sender: { + id: params.user.id, + name: params.user.username, + tag: formatDiscordUserTag(params.user), + }, + allowNameMatching: params.allowNameMatching, + }); + if (hasAccessRestrictions && !memberAllowed) { + return { allowed: false, reason: "guild-member-denied" }; + } return { allowed: true }; } @@ -434,9 +450,13 @@ async function handleDiscordReactionEvent( channelType === ChannelType.PublicThread || channelType === ChannelType.PrivateThread || channelType === ChannelType.AnnouncementThread; + const memberRoleIds = Array.isArray(data.rawMember?.roles) + ? data.rawMember.roles.map((roleId: string) => String(roleId)) + : []; const reactionIngressBase: Omit = { accountId: params.accountId, user, + memberRoleIds, isDirectMessage, isGroupDm, isGuildMessage, @@ -452,17 +472,18 @@ async function handleDiscordReactionEvent( allowNameMatching: params.allowNameMatching, guildInfo, }; - const ingressAccess = await authorizeDiscordReactionIngress(reactionIngressBase); - if (!ingressAccess.allowed) { - logVerbose(`discord reaction blocked sender=${user.id} (reason=${ingressAccess.reason})`); - return; + // Guild reactions need resolved channel/thread config before member access + // can mirror the normal message preflight path. + if (!isGuildMessage) { + const ingressAccess = await authorizeDiscordReactionIngress(reactionIngressBase); + if (!ingressAccess.allowed) { + logVerbose(`discord reaction blocked sender=${user.id} (reason=${ingressAccess.reason})`); + return; + } } let parentId = "parentId" in channel ? (channel.parentId ?? undefined) : undefined; let parentName: string | undefined; let parentSlug = ""; - const memberRoleIds = Array.isArray(data.rawMember?.roles) - ? data.rawMember.roles.map((roleId: string) => String(roleId)) - : []; let reactionBase: { baseText: string; contextKey: string } | null = null; const resolveReactionBase = () => { if (reactionBase) { @@ -507,6 +528,7 @@ async function handleDiscordReactionEvent( const shouldNotifyReaction = (options: { mode: "off" | "own" | "all" | "allowlist"; messageAuthorId?: string; + channelConfig?: ReturnType; }) => shouldEmitDiscordReactionNotification({ mode: options.mode, @@ -515,7 +537,9 @@ async function handleDiscordReactionEvent( userId: user.id, userName: user.username, userTag: formatDiscordUserTag(user), - allowlist: guildInfo?.users, + channelConfig: options.channelConfig, + guildInfo, + memberRoleIds, allowNameMatching: params.allowNameMatching, }); const emitReactionWithAuthor = (message: { author?: User } | null) => { @@ -550,10 +574,12 @@ async function handleDiscordReactionEvent( ...reactionIngressBase, channelConfig, }); - const authorizeThreadChannelAccess = async (channelInfo: { parentId?: string } | null) => { + const resolveThreadChannelAccess = async (channelInfo: { parentId?: string } | null) => { parentId = channelInfo?.parentId; await loadThreadParentInfo(); - return await authorizeReactionIngressForChannel(resolveThreadChannelConfig()); + const channelConfig = resolveThreadChannelConfig(); + const access = await authorizeReactionIngressForChannel(channelConfig); + return { access, channelConfig }; }; // Parallelize async operations for thread channels @@ -572,16 +598,18 @@ async function handleDiscordReactionEvent( // Fast path: for "all" and "allowlist" modes, we don't need to fetch the message if (reactionMode === "all" || reactionMode === "allowlist") { const channelInfo = await channelInfoPromise; - const threadAccess = await authorizeThreadChannelAccess(channelInfo); + const { access: threadAccess, channelConfig: threadChannelConfig } = + await resolveThreadChannelAccess(channelInfo); if (!threadAccess.allowed) { return; } - - // For allowlist mode, check if user is in allowlist first - if (reactionMode === "allowlist") { - if (!shouldNotifyReaction({ mode: reactionMode })) { - return; - } + if ( + !shouldNotifyReaction({ + mode: reactionMode, + channelConfig: threadChannelConfig, + }) + ) { + return; } const { baseText } = resolveReactionBase(); @@ -593,13 +621,20 @@ async function handleDiscordReactionEvent( const messagePromise = data.message.fetch().catch(() => null); const [channelInfo, message] = await Promise.all([channelInfoPromise, messagePromise]); - const threadAccess = await authorizeThreadChannelAccess(channelInfo); + const { access: threadAccess, channelConfig: threadChannelConfig } = + await resolveThreadChannelAccess(channelInfo); if (!threadAccess.allowed) { return; } const messageAuthorId = message?.author?.id ?? undefined; - if (!shouldNotifyReaction({ mode: reactionMode, messageAuthorId })) { + if ( + !shouldNotifyReaction({ + mode: reactionMode, + messageAuthorId, + channelConfig: threadChannelConfig, + }) + ) { return; } @@ -634,11 +669,8 @@ async function handleDiscordReactionEvent( // Fast path: for "all" and "allowlist" modes, we don't need to fetch the message if (reactionMode === "all" || reactionMode === "allowlist") { - // For allowlist mode, check if user is in allowlist first - if (reactionMode === "allowlist") { - if (!shouldNotifyReaction({ mode: reactionMode })) { - return; - } + if (!shouldNotifyReaction({ mode: reactionMode, channelConfig })) { + return; } const { baseText } = resolveReactionBase(); @@ -649,7 +681,7 @@ async function handleDiscordReactionEvent( // For "own" mode, we need to fetch the message to check the author const message = await data.message.fetch().catch(() => null); const messageAuthorId = message?.author?.id ?? undefined; - if (!shouldNotifyReaction({ mode: reactionMode, messageAuthorId })) { + if (!shouldNotifyReaction({ mode: reactionMode, messageAuthorId, channelConfig })) { return; }