refactor: unify reaction ingress policy guards across channels

2026-05-07 07:41:36 +00:00 · 2026-02-26 01:34:37 +01:00
parent 876018f322
commit 8f8e46d898
6 changed files with 289 additions and 188 deletions
--- 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";
--- 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";
--- 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({
--- 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 (