From b40c679630d65eae6741c225cca0c3bf7b364029 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 30 Apr 2026 15:49:34 +0100 Subject: [PATCH] fix(signal): match group allowlists against group ids --- CHANGELOG.md | 1 + docs/channels/groups.md | 1 + docs/channels/signal.md | 4 +- .../signal/src/monitor/access-policy.test.ts | 102 ++++++++- .../signal/src/monitor/access-policy.ts | 16 +- .../event-handler.inbound-context.test.ts | 205 ++++++++++++++++-- .../signal/src/monitor/event-handler.ts | 28 ++- 7 files changed, 322 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 822b554ae72..61abd167cd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai - Plugins/runtime-deps: keep bundled provider policy config loading from staging plugin runtime dependencies, so config reads no longer fail on locked-down `/var/lib/openclaw/plugin-runtime-deps` directories. Fixes #74971. Thanks @eurojojo. - Memory/runtime-deps: retain the native `node-llama-cpp` runtime only when local memory search is configured, so packaged installs can repair local embeddings without relying on unreachable global npm installs. Fixes #74777. Thanks @LLagoon3. - Gateway/startup: skip pre-bind web-fetch provider discovery for credential-free `tools.web.fetch` config, so Docker/Kubernetes gateways bind even when optional fetch limits are present. Fixes #74896. Thanks @KoykL. +- Signal: match group allowlists against inbound Signal group ids as well as sender ids, so configured groups no longer drop every message before mention policy runs. Part of #53308. Thanks @minupla and @juan-flores077. - Signal: bound `signal-cli` installer release and archive downloads with explicit timeouts, declared and streamed size checks, and partial-file cleanup. Fixes #54153. Thanks @jinduwang1001-max and @juan-flores077. - Slack: require bot-authored room messages with `allowBots=true` to come from an explicitly channel-allowlisted bot or from a room where an explicit Slack owner is present, so broad bot relays cannot run unattended. Fixes #59284. Thanks @andrewhong-translucent. - Signal: derive `getAttachment` HTTP response caps from `channels.signal.mediaMaxMb` with base64 headroom, so inbound photos and videos no longer drop behind the 1 MiB RPC default. Fixes #73564. Thanks @heyhudson. diff --git a/docs/channels/groups.md b/docs/channels/groups.md index 93330b4b6bf..3fb4669eb6b 100644 --- a/docs/channels/groups.md +++ b/docs/channels/groups.md @@ -257,6 +257,7 @@ Control how group/room messages are handled per channel: - `groupPolicy` is separate from mention-gating (which requires @mentions). - WhatsApp/Telegram/Signal/iMessage/Microsoft Teams/Zalo: use `groupAllowFrom` (fallback: explicit `allowFrom`). + - Signal: `groupAllowFrom` can match either the inbound Signal group id or the sender phone/UUID. - DM pairing approvals (`*-allowFrom` store entries) apply to DM access only; group sender authorization stays explicit to group allowlists. - Discord: allowlist uses `channels.discord.guilds..channels`. - Slack: allowlist uses `channels.slack.channels`. diff --git a/docs/channels/signal.md b/docs/channels/signal.md index f2ab951a865..c598695e02c 100644 --- a/docs/channels/signal.md +++ b/docs/channels/signal.md @@ -194,7 +194,7 @@ DMs: Groups: - `channels.signal.groupPolicy = open | allowlist | disabled`. -- `channels.signal.groupAllowFrom` controls who can trigger in groups when `allowlist` is set. +- `channels.signal.groupAllowFrom` controls which groups or senders can trigger group replies when `allowlist` is set; entries can be Signal group IDs (raw, `group:`, or `signal:group:`), sender phone numbers, or `uuid:` values. - `channels.signal.groups["" | "*"]` can override group behavior with `requireMention`, `tools`, and `toolsBySender`. - Use `channels.signal.accounts..groups` for per-account overrides in multi-account setups. - Runtime note: if `channels.signal` is completely missing, runtime falls back to `groupPolicy="allowlist"` for group checks (even if `channels.defaults.groupPolicy` is set). @@ -314,7 +314,7 @@ Provider options: - `channels.signal.dmPolicy`: `pairing | allowlist | open | disabled` (default: pairing). - `channels.signal.allowFrom`: DM allowlist (E.164 or `uuid:`). `open` requires `"*"`. Signal has no usernames; use phone/UUID ids. - `channels.signal.groupPolicy`: `open | allowlist | disabled` (default: allowlist). -- `channels.signal.groupAllowFrom`: group sender allowlist. +- `channels.signal.groupAllowFrom`: group allowlist; accepts Signal group IDs (raw, `group:`, or `signal:group:`), sender E.164 numbers, or `uuid:` values. - `channels.signal.groups`: per-group overrides keyed by Signal group id (or `"*"`). Supported fields: `requireMention`, `tools`, `toolsBySender`. - `channels.signal.accounts..groups`: per-account version of `channels.signal.groups` for multi-account setups. - `channels.signal.historyLimit`: max group messages to include as context (0 disables). diff --git a/extensions/signal/src/monitor/access-policy.test.ts b/extensions/signal/src/monitor/access-policy.test.ts index f057f4cdf05..fc7a8ad1555 100644 --- a/extensions/signal/src/monitor/access-policy.test.ts +++ b/extensions/signal/src/monitor/access-policy.test.ts @@ -1,5 +1,105 @@ import { describe, expect, it, vi } from "vitest"; -import { handleSignalDirectMessageAccess } from "./access-policy.js"; +import { handleSignalDirectMessageAccess, resolveSignalAccessState } from "./access-policy.js"; + +vi.mock("openclaw/plugin-sdk/security-runtime", async (importOriginal) => ({ + ...(await importOriginal()), + readStoreAllowFromForDmPolicy: vi.fn(async () => []), +})); + +const SIGNAL_GROUP_ID = "signal-group-id"; +const OTHER_SIGNAL_GROUP_ID = "other-signal-group-id"; +const SIGNAL_SENDER = { + kind: "phone" as const, + e164: "+15551230000", + raw: "+15551230000", +}; + +async function resolveGroupAccess(params: { + allowFrom?: string[]; + groupAllowFrom?: string[]; + groupId?: string; +}) { + const access = await resolveSignalAccessState({ + accountId: "default", + dmPolicy: "allowlist", + groupPolicy: "allowlist", + allowFrom: params.allowFrom ?? [], + groupAllowFrom: params.groupAllowFrom ?? [], + sender: SIGNAL_SENDER, + groupId: params.groupId, + }); + return { + ...access, + groupDecision: access.resolveAccessDecision(true), + }; +} + +describe("resolveSignalAccessState", () => { + it("allows group messages when groupAllowFrom contains the inbound Signal group id", async () => { + const { groupDecision } = await resolveGroupAccess({ + groupAllowFrom: [SIGNAL_GROUP_ID], + groupId: SIGNAL_GROUP_ID, + }); + + expect(groupDecision.decision).toBe("allow"); + }); + + it("allows Signal group target forms in groupAllowFrom", async () => { + const groupTargetDecision = await resolveGroupAccess({ + groupAllowFrom: [`group:${SIGNAL_GROUP_ID}`], + groupId: SIGNAL_GROUP_ID, + }); + const signalGroupTargetDecision = await resolveGroupAccess({ + groupAllowFrom: [`signal:group:${SIGNAL_GROUP_ID}`], + groupId: SIGNAL_GROUP_ID, + }); + + expect(groupTargetDecision.groupDecision.decision).toBe("allow"); + expect(signalGroupTargetDecision.groupDecision.decision).toBe("allow"); + }); + + it("blocks group messages when groupAllowFrom contains a different Signal group id", async () => { + const { groupDecision } = await resolveGroupAccess({ + groupAllowFrom: [OTHER_SIGNAL_GROUP_ID], + groupId: SIGNAL_GROUP_ID, + }); + + expect(groupDecision.decision).toBe("block"); + }); + + it("keeps sender allowlist compatibility for Signal group messages", async () => { + const { groupDecision } = await resolveGroupAccess({ + groupAllowFrom: [SIGNAL_SENDER.e164], + groupId: SIGNAL_GROUP_ID, + }); + + expect(groupDecision.decision).toBe("allow"); + }); + + it("does not match group ids against direct-message allowFrom entries", async () => { + const { dmAccess } = await resolveSignalAccessState({ + accountId: "default", + dmPolicy: "allowlist", + groupPolicy: "allowlist", + allowFrom: [SIGNAL_GROUP_ID], + groupAllowFrom: [], + sender: SIGNAL_SENDER, + groupId: SIGNAL_GROUP_ID, + }); + + expect(dmAccess.decision).toBe("block"); + }); + + it("does not let group ids in allowFrom satisfy an explicit groupAllowFrom mismatch", async () => { + const { groupDecision } = await resolveGroupAccess({ + allowFrom: [SIGNAL_GROUP_ID], + groupAllowFrom: [OTHER_SIGNAL_GROUP_ID], + groupId: SIGNAL_GROUP_ID, + }); + + expect(groupDecision.decision).toBe("block"); + }); +}); describe("handleSignalDirectMessageAccess", () => { it("returns true for already-allowed direct messages", async () => { diff --git a/extensions/signal/src/monitor/access-policy.ts b/extensions/signal/src/monitor/access-policy.ts index cf1aff2cbe4..5fdfdba0787 100644 --- a/extensions/signal/src/monitor/access-policy.ts +++ b/extensions/signal/src/monitor/access-policy.ts @@ -9,6 +9,14 @@ import { isSignalSenderAllowed, type SignalSender } from "../identity.js"; type SignalDmPolicy = "open" | "pairing" | "allowlist" | "disabled"; type SignalGroupPolicy = "open" | "allowlist" | "disabled"; +function isSignalGroupAllowed(groupId: string | undefined, allowEntries: string[]): boolean { + if (!groupId) { + return false; + } + const candidates = new Set([groupId, `group:${groupId}`, `signal:group:${groupId}`]); + return allowEntries.some((entry) => candidates.has(entry)); +} + export async function resolveSignalAccessState(params: { accountId: string; dmPolicy: SignalDmPolicy; @@ -16,12 +24,17 @@ export async function resolveSignalAccessState(params: { allowFrom: string[]; groupAllowFrom: string[]; sender: SignalSender; + groupId?: string; }) { const storeAllowFrom = await readStoreAllowFromForDmPolicy({ provider: "signal", accountId: params.accountId, dmPolicy: params.dmPolicy, }); + const isSenderAllowed = (allowEntries: string[]) => + isSignalSenderAllowed(params.sender, allowEntries); + const isSenderOrGroupAllowed = (allowEntries: string[]) => + isSenderAllowed(allowEntries) || isSignalGroupAllowed(params.groupId, allowEntries); const resolveAccessDecision = (isGroup: boolean) => resolveDmGroupAccessWithLists({ isGroup, @@ -30,11 +43,12 @@ export async function resolveSignalAccessState(params: { allowFrom: params.allowFrom, groupAllowFrom: params.groupAllowFrom, storeAllowFrom, - isSenderAllowed: (allowEntries) => isSignalSenderAllowed(params.sender, allowEntries), + isSenderAllowed: isGroup ? isSenderOrGroupAllowed : isSenderAllowed, }); const dmAccess = resolveAccessDecision(false); return { resolveAccessDecision, + isGroupAllowed: isSenderOrGroupAllowed, dmAccess, effectiveDmAllow: dmAccess.effectiveAllowFrom, effectiveGroupAllow: dmAccess.effectiveGroupAllowFrom, diff --git a/extensions/signal/src/monitor/event-handler.inbound-context.test.ts b/extensions/signal/src/monitor/event-handler.inbound-context.test.ts index 49dbab8d8ab..cf7c7e6388c 100644 --- a/extensions/signal/src/monitor/event-handler.inbound-context.test.ts +++ b/extensions/signal/src/monitor/event-handler.inbound-context.test.ts @@ -1,32 +1,38 @@ import { expectChannelInboundContextContract as expectInboundContextContract } from "openclaw/plugin-sdk/channel-contract-testing"; import type { MsgContext } from "openclaw/plugin-sdk/reply-runtime"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { SignalReactionMessage } from "./event-handler.types.js"; vi.useRealTimers(); const [ { createBaseSignalEventHandlerDeps, createSignalReceiveEvent }, { createSignalEventHandler }, ] = await Promise.all([import("./event-handler.test-harness.js"), import("./event-handler.js")]); -const { sendTypingMock, sendReadReceiptMock, dispatchInboundMessageMock, capture } = vi.hoisted( - () => { - const captureState: { ctx?: MsgContext } = {}; - return { - sendTypingMock: vi.fn(), - sendReadReceiptMock: vi.fn(), - dispatchInboundMessageMock: vi.fn( - async (params: { - ctx: MsgContext; - replyOptions?: { onReplyStart?: () => void | Promise }; - }) => { - captureState.ctx = params.ctx; - await Promise.resolve(params.replyOptions?.onReplyStart?.()); - return { queuedFinal: false, counts: { tool: 0, block: 0, final: 0 } }; - }, - ), - capture: captureState, - }; - }, -); +const { + sendTypingMock, + sendReadReceiptMock, + dispatchInboundMessageMock, + enqueueSystemEventMock, + capture, +} = vi.hoisted(() => { + const captureState: { ctx?: MsgContext } = {}; + return { + sendTypingMock: vi.fn(), + sendReadReceiptMock: vi.fn(), + enqueueSystemEventMock: vi.fn(), + dispatchInboundMessageMock: vi.fn( + async (params: { + ctx: MsgContext; + replyOptions?: { onReplyStart?: () => void | Promise }; + }) => { + captureState.ctx = params.ctx; + await Promise.resolve(params.replyOptions?.onReplyStart?.()); + return { queuedFinal: false, counts: { tool: 0, block: 0, final: 0 } }; + }, + ), + capture: captureState, + }; +}); vi.mock("../send.js", () => ({ sendMessageSignal: vi.fn(), @@ -57,11 +63,22 @@ vi.mock("openclaw/plugin-sdk/conversation-runtime", async () => { }; }); +vi.mock("openclaw/plugin-sdk/system-event-runtime", async () => { + const actual = await vi.importActual( + "openclaw/plugin-sdk/system-event-runtime", + ); + return { + ...actual, + enqueueSystemEvent: enqueueSystemEventMock, + }; +}); + describe("signal createSignalEventHandler inbound context", () => { beforeEach(() => { delete capture.ctx; sendTypingMock.mockReset().mockResolvedValue(true); sendReadReceiptMock.mockReset().mockResolvedValue(true); + enqueueSystemEventMock.mockReset(); dispatchInboundMessageMock.mockClear(); }); @@ -197,6 +214,154 @@ describe("signal createSignalEventHandler inbound context", () => { expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); }); + it("allows Signal groups whose id is listed in groupAllowFrom", async () => { + const handler = createSignalEventHandler( + createBaseSignalEventHandlerDeps({ + cfg: { + messages: { inbound: { debounceMs: 0 } }, + channels: { + signal: { + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + groups: { "*": { requireMention: false } }, + }, + }, + }, + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + historyLimit: 0, + }), + ); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + message: "hello from allowed group", + groupInfo: { groupId: "g1", groupName: "Test Group" }, + attachments: [], + }, + }), + ); + + expect(capture.ctx).toBeTruthy(); + expect(capture.ctx?.ChatType).toBe("group"); + expect(capture.ctx?.From).toBe("group:g1"); + }); + + it("blocks Signal groups whose id is not listed in groupAllowFrom", async () => { + const handler = createSignalEventHandler( + createBaseSignalEventHandlerDeps({ + cfg: { + messages: { inbound: { debounceMs: 0 } }, + channels: { + signal: { + groupPolicy: "allowlist", + groupAllowFrom: ["g2"], + groups: { "*": { requireMention: false } }, + }, + }, + }, + groupPolicy: "allowlist", + groupAllowFrom: ["g2"], + historyLimit: 0, + }), + ); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + message: "hello from blocked group", + groupInfo: { groupId: "g1", groupName: "Test Group" }, + attachments: [], + }, + }), + ); + + expect(capture.ctx).toBeUndefined(); + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + }); + + it("authorizes group control commands when groupAllowFrom matches the Signal group id", async () => { + const handler = createSignalEventHandler( + createBaseSignalEventHandlerDeps({ + cfg: { + messages: { + inbound: { debounceMs: 0 }, + groupChat: { mentionPatterns: ["@bot"] }, + }, + channels: { + signal: { + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + groups: { "*": { requireMention: true } }, + }, + }, + }, + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + historyLimit: 0, + }), + ); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + message: "/status", + groupInfo: { groupId: "g1", groupName: "Test Group" }, + attachments: [], + }, + }), + ); + + expect(capture.ctx).toBeTruthy(); + expect(capture.ctx?.CommandAuthorized).toBe(true); + }); + + it("allows reaction-only group events when groupAllowFrom matches the reaction group id", async () => { + const handler = createSignalEventHandler( + createBaseSignalEventHandlerDeps({ + cfg: { + messages: { inbound: { debounceMs: 0 } }, + channels: { + signal: { + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + }, + }, + }, + groupPolicy: "allowlist", + groupAllowFrom: ["g1"], + reactionMode: "all", + isSignalReactionMessage: (reaction): reaction is SignalReactionMessage => Boolean(reaction), + shouldEmitSignalReactionNotification: () => true, + resolveSignalReactionTargets: () => [ + { kind: "phone", id: "+15550001111", display: "+15550001111" }, + ], + buildSignalReactionSystemEventText: () => "reaction added", + historyLimit: 0, + }), + ); + + await handler( + createSignalReceiveEvent({ + reactionMessage: { + emoji: "+1", + targetSentTimestamp: 1700000000000, + groupInfo: { groupId: "g1", groupName: "Test Group" }, + }, + }), + ); + + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledWith( + "reaction added", + expect.objectContaining({ + sessionKey: "agent:main:signal:group:g1", + trusted: false, + }), + ); + }); + it("drops quote-only group context from non-allowlisted quoted senders in allowlist mode", async () => { const handler = createSignalEventHandler( createBaseSignalEventHandlerDeps({ diff --git a/extensions/signal/src/monitor/event-handler.ts b/extensions/signal/src/monitor/event-handler.ts index d87764630a1..8276216d02b 100644 --- a/extensions/signal/src/monitor/event-handler.ts +++ b/extensions/signal/src/monitor/event-handler.ts @@ -552,19 +552,25 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const rawMessage = dataMessage?.message ?? ""; const normalizedMessage = renderSignalMentions(rawMessage, dataMessage?.mentions); const messageText = normalizedMessage.trim(); - const groupId = dataMessage?.groupInfo?.groupId ?? undefined; + const groupId = dataMessage?.groupInfo?.groupId ?? reaction?.groupInfo?.groupId ?? undefined; const isGroup = Boolean(groupId); const senderDisplay = formatSignalSenderDisplay(sender); - const { resolveAccessDecision, dmAccess, effectiveDmAllow, effectiveGroupAllow } = - await resolveSignalAccessState({ - accountId: deps.accountId, - dmPolicy: deps.dmPolicy, - groupPolicy: deps.groupPolicy, - allowFrom: deps.allowFrom, - groupAllowFrom: deps.groupAllowFrom, - sender, - }); + const { + resolveAccessDecision, + isGroupAllowed, + dmAccess, + effectiveDmAllow, + effectiveGroupAllow, + } = await resolveSignalAccessState({ + accountId: deps.accountId, + dmPolicy: deps.dmPolicy, + groupPolicy: deps.groupPolicy, + allowFrom: deps.allowFrom, + groupAllowFrom: deps.groupAllowFrom, + sender, + groupId, + }); const quoteText = normalizeOptionalString(dataMessage?.quote?.text) ?? ""; const { contextVisibilityMode, quoteSenderAllowed, visibleQuoteText, visibleQuoteSender } = resolveSignalQuoteContext({ @@ -650,7 +656,7 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const useAccessGroups = deps.cfg.commands?.useAccessGroups !== false; const commandDmAllow = isGroup ? deps.allowFrom : effectiveDmAllow; const ownerAllowedForCommands = isSignalSenderAllowed(sender, commandDmAllow); - const groupAllowedForCommands = isSignalSenderAllowed(sender, effectiveGroupAllow); + const groupAllowedForCommands = isGroupAllowed(effectiveGroupAllow); const hasControlCommandInMessage = hasControlCommand(messageText, deps.cfg); const commandGate = resolveControlCommandGate({ useAccessGroups,