From b7e0decf0cd3cef742c02b9e8ad58cefd3c8c3a9 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Wed, 13 May 2026 07:16:27 +0530 Subject: [PATCH] Restrict chat sender allowlist matching [AI] (#80898) * fix: restrict chat sender allowlist matching * fix: restrict chat sender allowlist matching * addressing codex review * fix: complete sender allowlist root cause * addressing codex review * addressing codex review * fix: complete root-cause handling * addressing review-skill * addressing codex review * addressing review-skill * addressing codex review * addressing codex review * fix: complete chat sender allowlist handling * addressing codex review * fix: complete root-cause handling * addressing codex review * fix: complete root-cause handling * addressing codex review * fix: cover sender matcher conversation target opt-in * addressing review-skill * addressing codex review * fix: require explicit chat target sender matching * addressing review-skill * addressing codex review * addressing codex review * fix: require explicit chat target sender matching * addressing codex review * fix: require explicit chat target sender matching * addressing codex review * addressing codex review * fix: require explicit chat target sender matching * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + docs/channels/imessage.md | 4 +- .../imessage/src/monitor.gating.test.ts | 243 ++++++++++++++++++ .../src/monitor/inbound-processing.ts | 65 ++++- .../imessage/src/monitor/monitor-provider.ts | 6 +- extensions/imessage/src/setup-core.ts | 58 +++-- extensions/imessage/src/targets.test.ts | 85 +++++- extensions/imessage/src/targets.ts | 15 +- .../plugins/chat-target-prefixes.test.ts | 120 +++++++++ src/channels/plugins/chat-target-prefixes.ts | 14 +- src/plugin-sdk/allow-from.test.ts | 68 ++++- src/plugin-sdk/allow-from.ts | 3 +- 12 files changed, 624 insertions(+), 58 deletions(-) create mode 100644 src/channels/plugins/chat-target-prefixes.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a6bfd8c90e4..e7021657cd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Restrict chat sender allowlist matching [AI]. (#80898) Thanks @pgondhi987. - Sessions: redact persisted tool result detail metadata before writing transcripts so diagnostic secrets do not survive tool output redaction. (#80444) Thanks @nimbleenigma. - Codex migration: make Enter activate the highlighted checkbox row before continuing, so `Skip for now` and bulk-selection rows work even when planned items start preselected. - fix: harden safe-bin argument validation [AI]. (#80999) Thanks @pgondhi987. diff --git a/docs/channels/imessage.md b/docs/channels/imessage.md index e1c54d2ad76..c63d56f5f6f 100644 --- a/docs/channels/imessage.md +++ b/docs/channels/imessage.md @@ -217,7 +217,7 @@ If SIP-disabled isn't acceptable for your threat model: Allowlist field: `channels.imessage.allowFrom`. - Allowlist entries can be handles, static sender access groups (`accessGroup:`), or chat targets (`chat_id:*`, `chat_guid:*`, `chat_identifier:*`). + Allowlist entries must identify senders: handles or static sender access groups (`accessGroup:`). Use `channels.imessage.groupAllowFrom` for chat targets such as `chat_id:*`, `chat_guid:*`, or `chat_identifier:*`; use `channels.imessage.groups` for numeric `chat_id` registry keys. @@ -232,7 +232,7 @@ If SIP-disabled isn't acceptable for your threat model: `groupAllowFrom` entries can also reference static sender access groups (`accessGroup:`). - Runtime fallback: if `groupAllowFrom` is unset, iMessage group sender checks fall back to `allowFrom` when available. + Runtime fallback: if `groupAllowFrom` is unset, iMessage group sender checks use `allowFrom`; set `groupAllowFrom` when DM and group admission should differ. Runtime note: if `channels.imessage` is completely missing, runtime falls back to `groupPolicy="allowlist"` and logs a warning (even if `channels.defaults.groupPolicy` is set). diff --git a/extensions/imessage/src/monitor.gating.test.ts b/extensions/imessage/src/monitor.gating.test.ts index 228f2192398..16e2bf1436a 100644 --- a/extensions/imessage/src/monitor.gating.test.ts +++ b/extensions/imessage/src/monitor.gating.test.ts @@ -59,6 +59,7 @@ async function resolveDispatchDecision(params: { groupHistories?: Parameters[0]["groupHistories"]; allowFrom?: string[]; groupAllowFrom?: string[]; + allowLegacyConversationAllowFromForGroup?: boolean; groupPolicy?: "open" | "allowlist" | "disabled"; dmPolicy?: "open" | "pairing" | "allowlist" | "disabled"; }) { @@ -72,6 +73,7 @@ async function resolveDispatchDecision(params: { bodyText: params.message.text ?? "", allowFrom: params.allowFrom ?? ["*"], groupAllowFrom: params.groupAllowFrom ?? [], + allowLegacyConversationAllowFromForGroup: params.allowLegacyConversationAllowFromForGroup, groupPolicy: params.groupPolicy ?? "open", dmPolicy: params.dmPolicy ?? "open", storeAllowFrom: [], @@ -253,6 +255,90 @@ describe("imessage monitor gating + envelope builders", () => { expect(ctxPayload.Body ?? "").not.toContain("[Replying to"); }); + it("keeps group reply context when the group allowlist matches the chat target", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + cfg.channels.imessage.contextVisibility = "allowlist"; + + const message: IMessagePayload = { + id: 8, + chat_id: 55, + sender: "+15550001111", + is_from_me: false, + text: "@openclaw replying now", + is_group: true, + reply_to_id: 9001, + reply_to_text: "quoted context", + reply_to_sender: "+15559998888", + }; + const { decision, groupHistories } = await resolveDispatchDecision({ + cfg, + message, + allowFrom: ["*"], + groupAllowFrom: ["chat_id:55"], + groupPolicy: "allowlist", + }); + const { ctxPayload } = buildIMessageInboundContext({ + cfg, + decision, + message, + historyLimit: 0, + groupHistories, + }); + + expect(ctxPayload.ReplyToId).toBe("9001"); + expect(ctxPayload.ReplyToBody).toBe("quoted context"); + expect(ctxPayload.ReplyToSender).toBe("+15559998888"); + expect(ctxPayload.Body ?? "").toContain("[Replying to +15559998888 id:9001]"); + }); + + it("keeps group reply context when the group allowlist matches an access group", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + cfg.channels.imessage.contextVisibility = "allowlist"; + cfg.accessGroups = { + oncall: { + type: "message.senders", + members: { imessage: ["+15559998888"] }, + }, + }; + + const message: IMessagePayload = { + id: 9, + chat_id: 56, + sender: "+15559998888", + is_from_me: false, + text: "@openclaw replying now", + is_group: true, + reply_to_id: 9002, + reply_to_text: "own quoted context", + reply_to_sender: "+15559998888", + }; + const { decision, groupHistories } = await resolveDispatchDecision({ + cfg, + message, + allowFrom: ["*"], + groupAllowFrom: ["accessGroup:oncall"], + groupPolicy: "allowlist", + }); + const { ctxPayload } = buildIMessageInboundContext({ + cfg, + decision, + message, + historyLimit: 0, + groupHistories, + }); + + expect(ctxPayload.ReplyToId).toBe("9002"); + expect(ctxPayload.ReplyToBody).toBe("own quoted context"); + expect(ctxPayload.ReplyToSender).toBe("+15559998888"); + expect(ctxPayload.Body ?? "").toContain("[Replying to +15559998888 id:9002]"); + }); + it("keeps group reply context in allowlist_quote mode", async () => { const cfg = baseCfg(); cfg.channels ??= {}; @@ -432,6 +518,163 @@ describe("imessage monitor gating + envelope builders", () => { expect(allowed.kind).toBe("dispatch"); }); + it("uses legacy conversation allowFrom entries for group admission", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + + const { decision } = await resolveDispatchDecision({ + cfg, + message: { + id: 35, + chat_id: 101, + sender: "+15550003333", + is_from_me: false, + text: "@openclaw ok", + is_group: true, + }, + allowFrom: ["chat_id:101"], + groupAllowFrom: [], + allowLegacyConversationAllowFromForGroup: true, + groupPolicy: "allowlist", + }); + + expect(decision.kind).toBe("dispatch"); + }); + + it("does not use legacy conversation allowFrom entries when groupAllowFrom is explicitly empty", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + + const decision = await resolveIMessageInboundDecision({ + cfg, + accountId: "default", + message: { + id: 38, + chat_id: 101, + sender: "+15550003333", + is_from_me: false, + text: "@openclaw ok", + is_group: true, + }, + opts: {}, + messageText: "@openclaw ok", + bodyText: "@openclaw ok", + allowFrom: ["chat_id:101"], + groupAllowFrom: [], + groupPolicy: "allowlist", + dmPolicy: "pairing", + storeAllowFrom: [], + historyLimit: 0, + groupHistories: new Map(), + }); + + expect(decision).toEqual({ + kind: "drop", + reason: "groupPolicy allowlist (empty groupAllowFrom)", + }); + }); + + it("does not merge legacy conversation allowFrom entries when groupAllowFrom is configured", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + + const decision = await resolveIMessageInboundDecision({ + cfg, + accountId: "default", + message: { + id: 37, + chat_id: 101, + sender: "+15550003333", + is_from_me: false, + text: "@openclaw ok", + is_group: true, + }, + opts: {}, + messageText: "@openclaw ok", + bodyText: "@openclaw ok", + allowFrom: ["chat_id:101"], + groupAllowFrom: ["+15550004444"], + groupPolicy: "allowlist", + dmPolicy: "pairing", + storeAllowFrom: [], + historyLimit: 0, + groupHistories: new Map(), + }); + + expect(decision).toEqual({ kind: "drop", reason: "not in groupAllowFrom" }); + }); + + it("does not authorize group control commands from conversation allowlist entries", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + + const decision = await resolveIMessageInboundDecision({ + cfg, + accountId: "default", + message: { + id: 34, + chat_id: 101, + sender: "+15550003333", + is_from_me: false, + text: "/status", + is_group: true, + }, + opts: {}, + messageText: "/status", + bodyText: "/status", + allowFrom: [], + groupAllowFrom: ["chat_id:101"], + groupPolicy: "allowlist", + dmPolicy: "pairing", + storeAllowFrom: [], + historyLimit: 0, + groupHistories: new Map(), + }); + + expect(decision).toEqual({ kind: "drop", reason: "control command (unauthorized)" }); + }); + + it("does not authorize group control commands from legacy conversation allowFrom entries", async () => { + const cfg = baseCfg(); + cfg.channels ??= {}; + cfg.channels.imessage ??= {}; + cfg.channels.imessage.groupPolicy = "allowlist"; + + const decision = await resolveIMessageInboundDecision({ + cfg, + accountId: "default", + message: { + id: 36, + chat_id: 101, + sender: "+15550003333", + is_from_me: false, + text: "/status", + is_group: true, + }, + opts: {}, + messageText: "/status", + bodyText: "/status", + allowFrom: ["chat_id:101"], + groupAllowFrom: [], + allowLegacyConversationAllowFromForGroup: true, + groupPolicy: "allowlist", + dmPolicy: "pairing", + storeAllowFrom: [], + historyLimit: 0, + groupHistories: new Map(), + }); + + expect(decision).toEqual({ kind: "drop", reason: "control command (unauthorized)" }); + }); + it("blocks group messages when groupPolicy is disabled", async () => { const cfg = baseCfg(); cfg.channels ??= {}; diff --git a/extensions/imessage/src/monitor/inbound-processing.ts b/extensions/imessage/src/monitor/inbound-processing.ts index 78107222a9b..1065c55eb4d 100644 --- a/extensions/imessage/src/monitor/inbound-processing.ts +++ b/extensions/imessage/src/monitor/inbound-processing.ts @@ -11,6 +11,7 @@ import { import { createChannelIngressResolver, defineStableChannelIngressIdentity, + type ChannelIngressIdentityDescriptor, } from "openclaw/plugin-sdk/channel-ingress-runtime"; import { resolveChannelGroupPolicy, @@ -36,7 +37,7 @@ import { } from "../monitor-reply-cache.js"; import { formatIMessageChatTarget, - isAllowedIMessageSender, + isAllowedIMessageReplyContextSender, normalizeIMessageHandle, parseIMessageAllowTarget, } from "../targets.js"; @@ -158,11 +159,52 @@ export function resolveIMessageReactionContext( const normalizeNonEmpty = (value: string) => value.trim() || null; +const imessageConversationIdentityKinds = new Set([ + "plugin:imessage-chat-id", + "plugin:imessage-chat-guid", + "plugin:imessage-chat-identifier", +]); + +const matchIMessageIngressEntry: NonNullable = ({ + entry, + context, +}) => { + if (imessageConversationIdentityKinds.has(entry.kind) && context !== "group") { + return false; + } + return undefined; +}; + +function isIMessageConversationAllowTarget(entry: string): boolean { + const parsed = parseIMessageAllowTarget(entry); + return ( + parsed.kind === "chat_id" || parsed.kind === "chat_guid" || parsed.kind === "chat_identifier" + ); +} + +function mergeIMessageGroupAllowFromWithLegacyChatTargets(params: { + groupAllowFrom: string[]; + allowFrom: string[]; + allowLegacyConversationTargets?: boolean; +}): string[] { + if (params.groupAllowFrom.length > 0 || !params.allowLegacyConversationTargets) { + return params.groupAllowFrom; + } + const legacyChatTargets = params.allowFrom.filter((entry) => + isIMessageConversationAllowTarget(entry), + ); + if (legacyChatTargets.length === 0) { + return params.groupAllowFrom; + } + return Array.from(new Set([...params.groupAllowFrom, ...legacyChatTargets])); +} + const imessageIngressIdentity = defineStableChannelIngressIdentity({ key: "imessage-sender", normalizeEntry: normalizeIMessageHandleEntry, normalizeSubject: normalizeIMessageHandle, sensitivity: "pii", + matchEntry: matchIMessageIngressEntry, aliases: ( [ ["imessage-chat-id", "plugin:imessage-chat-id", normalizeIMessageChatIdEntry], @@ -392,6 +434,7 @@ export async function resolveIMessageInboundDecision(params: { bodyText: string; allowFrom: string[]; groupAllowFrom: string[]; + allowLegacyConversationAllowFromForGroup?: boolean; groupPolicy: string; dmPolicy: string; storeAllowFrom: string[]; @@ -425,12 +468,18 @@ export async function resolveIMessageInboundDecision(params: { const reactionContext = resolveIMessageReactionContext(params.message, bodyText || messageText); const groupIdCandidate = chatId !== undefined ? String(chatId) : undefined; + const groupAllowFromWithLegacyChatTargets = mergeIMessageGroupAllowFromWithLegacyChatTargets({ + groupAllowFrom: params.groupAllowFrom, + allowFrom: params.allowFrom, + allowLegacyConversationTargets: params.allowLegacyConversationAllowFromForGroup, + }); const groupListPolicy = groupIdCandidate ? resolveChannelGroupPolicy({ cfg: params.cfg, channel: "imessage", accountId: params.accountId, groupId: groupIdCandidate, + hasGroupAllowFrom: groupAllowFromWithLegacyChatTargets.length > 0, }) : { allowlistEnabled: false, @@ -514,6 +563,9 @@ export async function resolveIMessageInboundDecision(params: { const groupId = isGroup ? groupIdCandidate : undefined; const hasControlCommandInMessage = hasControlCommand(messageText, params.cfg); + const groupAllowFromForAccess = isGroup + ? groupAllowFromWithLegacyChatTargets + : params.groupAllowFrom; const accessDecision = await createChannelIngressResolver({ channelId: "imessage", accountId: params.accountId, @@ -539,7 +591,7 @@ export async function resolveIMessageInboundDecision(params: { groupPolicy: normalizeGroupPolicy(params.groupPolicy), policy: { groupAllowFromFallbackToAllowFrom: false }, allowFrom: params.allowFrom, - groupAllowFrom: params.groupAllowFrom, + groupAllowFrom: groupAllowFromForAccess, command: { allowTextCommands: isGroup, hasControlCommand: hasControlCommandInMessage, @@ -718,12 +770,15 @@ export async function resolveIMessageInboundDecision(params: { channel: "imessage", accountId: params.accountId, }); + const replyContextAllowFrom = Array.from( + new Set([...groupAllowFromForAccess, ...effectiveGroupAllowFrom]), + ); const replySenderAllowed = - !isGroup || effectiveGroupAllowFrom.length === 0 + !isGroup || replyContextAllowFrom.length === 0 ? true : replyContext?.sender - ? isAllowedIMessageSender({ - allowFrom: effectiveGroupAllowFrom, + ? isAllowedIMessageReplyContextSender({ + allowFrom: replyContextAllowFrom, sender: replyContext.sender, chatId, chatGuid, diff --git a/extensions/imessage/src/monitor/monitor-provider.ts b/extensions/imessage/src/monitor/monitor-provider.ts index 656b84915a9..3fbd1c8d72e 100644 --- a/extensions/imessage/src/monitor/monitor-provider.ts +++ b/extensions/imessage/src/monitor/monitor-provider.ts @@ -172,11 +172,12 @@ export async function monitorIMessageProvider(opts: MonitorIMessageOpts = {}): P const loopRateLimiter = createLoopRateLimiter(); const textLimit = resolveTextChunkLimit(cfg, "imessage", accountInfo.accountId); const allowFrom = normalizeAllowList(opts.allowFrom ?? imessageCfg.allowFrom); + const configuredGroupAllowFrom = opts.groupAllowFrom ?? imessageCfg.groupAllowFrom; const groupAllowFrom = normalizeAllowList( - opts.groupAllowFrom ?? - imessageCfg.groupAllowFrom ?? + configuredGroupAllowFrom ?? (imessageCfg.allowFrom && imessageCfg.allowFrom.length > 0 ? imessageCfg.allowFrom : []), ); + const allowLegacyConversationAllowFromForGroup = configuredGroupAllowFrom == null; const defaultGroupPolicy = resolveDefaultGroupPolicy(cfg); const { groupPolicy, providerMissingFallbackApplied } = resolveOpenProviderRuntimeGroupPolicy({ providerConfigPresent: cfg.channels?.imessage !== undefined, @@ -374,6 +375,7 @@ export async function monitorIMessageProvider(opts: MonitorIMessageOpts = {}): P bodyText, allowFrom, groupAllowFrom, + allowLegacyConversationAllowFromForGroup, groupPolicy, dmPolicy, storeAllowFrom, diff --git a/extensions/imessage/src/setup-core.ts b/extensions/imessage/src/setup-core.ts index e3fe64d797c..a5be6aa5646 100644 --- a/extensions/imessage/src/setup-core.ts +++ b/extensions/imessage/src/setup-core.ts @@ -24,27 +24,39 @@ import { normalizeIMessageHandle } from "./targets.js"; const channel = "imessage" as const; +const CHAT_TARGET_ALLOWFROM_PREFIXES = [ + "chat_id:", + "chatid:", + "chat:", + "chat_guid:", + "chatguid:", + "guid:", + "chat_identifier:", + "chatidentifier:", + "chatident:", +]; +const SERVICE_ALLOWFROM_PREFIXES = ["imessage:", "sms:", "auto:"]; + +function normalizeAllowFromEntryForPrefixCheck(entry: string): string { + let lower = normalizeLowercaseStringOrEmpty(entry); + let stripped = true; + while (stripped) { + stripped = false; + for (const prefix of SERVICE_ALLOWFROM_PREFIXES) { + if (lower.startsWith(prefix)) { + lower = lower.slice(prefix.length).trim(); + stripped = true; + } + } + } + return lower; +} + export function parseIMessageAllowFromEntries(raw: string): { entries: string[]; error?: string } { return parseSetupEntriesAllowingWildcard(raw, (entry) => { - const lower = normalizeLowercaseStringOrEmpty(entry); - if (lower.startsWith("chat_id:")) { - const id = entry.slice("chat_id:".length).trim(); - if (!/^\d+$/.test(id)) { - return { error: `Invalid chat_id: ${entry}` }; - } - return { value: entry }; - } - if (lower.startsWith("chat_guid:")) { - if (!entry.slice("chat_guid:".length).trim()) { - return { error: "Invalid chat_guid entry" }; - } - return { value: entry }; - } - if (lower.startsWith("chat_identifier:")) { - if (!entry.slice("chat_identifier:".length).trim()) { - return { error: "Invalid chat_identifier entry" }; - } - return { value: entry }; + const lower = normalizeAllowFromEntryForPrefixCheck(entry); + if (CHAT_TARGET_ALLOWFROM_PREFIXES.some((prefix) => lower.startsWith(prefix))) { + return { error: `iMessage allowFrom entries must be sender handles: ${entry}` }; } if (!normalizeIMessageHandle(entry)) { return { error: `Invalid handle: ${entry}` }; @@ -79,17 +91,15 @@ async function promptIMessageAllowFrom(params: { prompter: params.prompter, noteTitle: "iMessage allowlist", noteLines: [ - "Allowlist iMessage DMs by handle or chat target.", + "Allowlist iMessage DMs by sender handle.", "Examples:", "- +15555550123", "- user@example.com", - "- chat_id:123", - "- chat_guid:... or chat_identifier:...", "Multiple entries: comma-separated.", `Docs: ${formatDocsLink("/imessage", "imessage")}`, ], - message: "iMessage allowFrom (handle or chat_id)", - placeholder: "+15555550123, user@example.com, chat_id:123", + message: "iMessage allowFrom (sender handle)", + placeholder: "+15555550123, user@example.com", parseEntries: parseIMessageAllowFromEntries, getExistingAllowFrom: ({ cfg, accountId }) => resolveIMessageAccount({ cfg, accountId }).config.allowFrom ?? [], diff --git a/extensions/imessage/src/targets.test.ts b/extensions/imessage/src/targets.test.ts index 18b3cf96ab1..74a00574db2 100644 --- a/extensions/imessage/src/targets.test.ts +++ b/extensions/imessage/src/targets.test.ts @@ -8,6 +8,7 @@ import { parseIMessageAllowFromEntries } from "./setup-surface.js"; import { formatIMessageChatTarget, inferIMessageTargetChatType, + isAllowedIMessageReplyContextSender, isAllowedIMessageSender, looksLikeIMessageExplicitTargetId, normalizeIMessageHandle, @@ -56,13 +57,46 @@ describe("imessage targets", () => { expect(normalizeIMessageHandle("CHATIDENT:foo")).toBe("chat_identifier:foo"); }); - it("checks allowFrom against chat_id", () => { + it("does not check allowFrom against conversation targets", () => { const ok = isAllowedIMessageSender({ allowFrom: ["chat_id:9"], sender: "+1555", chatId: 9, }); - expect(ok).toBe(true); + expect(ok).toBe(false); + + expect( + isAllowedIMessageSender({ + allowFrom: ["imessage:chat_id:9"], + sender: "+1555", + chatId: 9, + }), + ).toBe(false); + + expect( + isAllowedIMessageSender({ + allowFrom: ["chat_guid:team-thread"], + sender: "+1555", + chatGuid: "team-thread", + }), + ).toBe(false); + + expect( + isAllowedIMessageSender({ + allowFrom: ["chat_identifier:team"], + sender: "+1555", + chatIdentifier: "team", + }), + ).toBe(false); + + expect( + isAllowedIMessageSender({ + allowFrom: ["chat_id:9"], + sender: "+1555", + chatId: 9, + allowConversationTargets: true, + }), + ).toBe(false); }); it("checks allowFrom against handle", () => { @@ -73,6 +107,32 @@ describe("imessage targets", () => { expect(ok).toBe(true); }); + it("checks reply context allowFrom against conversation targets", () => { + expect( + isAllowedIMessageReplyContextSender({ + allowFrom: ["chat_id:9"], + sender: "+1555", + chatId: 9, + }), + ).toBe(true); + + expect( + isAllowedIMessageReplyContextSender({ + allowFrom: ["imessage:chat_guid:team-thread"], + sender: "+1555", + chatGuid: "team-thread", + }), + ).toBe(true); + + expect( + isAllowedIMessageReplyContextSender({ + allowFrom: ["chat_identifier:team"], + sender: "+1555", + chatIdentifier: "team", + }), + ).toBe(true); + }); + it("denies when allowFrom is empty", () => { const ok = isAllowedIMessageSender({ allowFrom: [], @@ -130,23 +190,28 @@ describe("imessage group policy", () => { }); describe("parseIMessageAllowFromEntries", () => { - it("parses handles and chat targets", () => { - expect(parseIMessageAllowFromEntries("+15555550123, chat_id:123, chat_guid:abc")).toEqual({ - entries: ["+15555550123", "chat_id:123", "chat_guid:abc"], + it("parses handles", () => { + expect(parseIMessageAllowFromEntries("+15555550123, user@example.com")).toEqual({ + entries: ["+15555550123", "user@example.com"], }); }); - it("returns validation errors for invalid chat_id", () => { - expect(parseIMessageAllowFromEntries("chat_id:abc")).toEqual({ + it("returns validation errors for chat target entries", () => { + expect(parseIMessageAllowFromEntries("chat_id:123")).toEqual({ entries: [], - error: "Invalid chat_id: chat_id:abc", + error: "iMessage allowFrom entries must be sender handles: chat_id:123", + }); + + expect(parseIMessageAllowFromEntries("imessage:chat_id:123")).toEqual({ + entries: [], + error: "iMessage allowFrom entries must be sender handles: imessage:chat_id:123", }); }); - it("returns validation errors for invalid chat_identifier entries", () => { + it("returns validation errors for chat_identifier entries", () => { expect(parseIMessageAllowFromEntries("chat_identifier:")).toEqual({ entries: [], - error: "Invalid chat_identifier entry", + error: "iMessage allowFrom entries must be sender handles: chat_identifier:", }); }); diff --git a/extensions/imessage/src/targets.ts b/extensions/imessage/src/targets.ts index ee9050842db..226db65bad1 100644 --- a/extensions/imessage/src/targets.ts +++ b/extensions/imessage/src/targets.ts @@ -1,8 +1,8 @@ import { normalizeE164 } from "openclaw/plugin-sdk/account-resolution"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/string-coerce-runtime"; import { - createAllowedChatSenderMatcher, type ChatSenderAllowParams, + createAllowedChatSenderMatcher, type ParsedChatTarget, parseChatTargetPrefixesOrThrow, resolveServicePrefixedChatTarget, @@ -162,10 +162,21 @@ export function parseIMessageAllowTarget(raw: string): IMessageAllowTarget { const isAllowedIMessageSenderMatcher = createAllowedChatSenderMatcher({ normalizeSender: normalizeIMessageHandle, parseAllowTarget: parseIMessageAllowTarget, + allowConversationTargets: false, }); export function isAllowedIMessageSender(params: ChatSenderAllowParams): boolean { - return isAllowedIMessageSenderMatcher(params); + return isAllowedIMessageSenderMatcher({ ...params, allowConversationTargets: false }); +} + +const isAllowedIMessageReplyContextSenderMatcher = createAllowedChatSenderMatcher({ + normalizeSender: normalizeIMessageHandle, + parseAllowTarget: parseIMessageAllowTarget, + allowConversationTargets: true, +}); + +export function isAllowedIMessageReplyContextSender(params: ChatSenderAllowParams): boolean { + return isAllowedIMessageReplyContextSenderMatcher(params); } export function formatIMessageChatTarget(chatId?: number | null): string { diff --git a/src/channels/plugins/chat-target-prefixes.test.ts b/src/channels/plugins/chat-target-prefixes.test.ts new file mode 100644 index 00000000000..1d28232b77e --- /dev/null +++ b/src/channels/plugins/chat-target-prefixes.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it } from "vitest"; +import { + createAllowedChatSenderMatcher, + isAllowedParsedChatSender, + type ParsedChatAllowTarget, +} from "./chat-target-prefixes.js"; + +function normalizeSender(sender: string): string { + return sender.trim().toLowerCase(); +} + +function parseAllowTarget(entry: string): ParsedChatAllowTarget { + const trimmed = entry.trim(); + const lower = trimmed.toLowerCase(); + if (lower.startsWith("chat_id:")) { + return { kind: "chat_id", chatId: Number.parseInt(trimmed.slice("chat_id:".length), 10) }; + } + if (lower.startsWith("chat_guid:")) { + return { kind: "chat_guid", chatGuid: trimmed.slice("chat_guid:".length).trim() }; + } + if (lower.startsWith("chat_identifier:")) { + return { + kind: "chat_identifier", + chatIdentifier: trimmed.slice("chat_identifier:".length).trim(), + }; + } + return { kind: "handle", handle: normalizeSender(trimmed) }; +} + +describe("isAllowedParsedChatSender", () => { + it("matches wildcard and normalized sender handles", () => { + expect( + isAllowedParsedChatSender({ + allowFrom: ["owner@example.com"], + sender: "Owner@Example.com", + normalizeSender, + parseAllowTarget, + }), + ).toBe(true); + + expect( + isAllowedParsedChatSender({ + allowFrom: ["*"], + sender: "other@example.com", + normalizeSender, + parseAllowTarget, + }), + ).toBe(true); + }); + + it("does not match conversation targets unless explicitly enabled", () => { + for (const entry of ["chat_id:123", "chat_guid:thread-123", "chat_identifier:team"]) { + expect( + isAllowedParsedChatSender({ + allowFrom: [entry], + sender: "other@example.com", + chatId: 123, + chatGuid: "thread-123", + chatIdentifier: "team", + normalizeSender, + parseAllowTarget, + }), + ).toBe(false); + + expect( + isAllowedParsedChatSender({ + allowFrom: [entry], + sender: "other@example.com", + chatId: 123, + chatGuid: "thread-123", + chatIdentifier: "team", + allowConversationTargets: true, + normalizeSender, + parseAllowTarget, + }), + ).toBe(true); + } + }); +}); + +describe("createAllowedChatSenderMatcher", () => { + it("keeps conversation targets disabled unless the matcher opts in", () => { + const matcher = createAllowedChatSenderMatcher({ + normalizeSender, + parseAllowTarget, + }); + + for (const entry of ["chat_id:123", "chat_guid:thread-123", "chat_identifier:team"]) { + expect( + matcher({ + allowFrom: [entry], + sender: "other@example.com", + chatId: 123, + chatGuid: "thread-123", + chatIdentifier: "team", + }), + ).toBe(false); + } + }); + + it("matches conversation targets when the matcher explicitly opts in", () => { + const matcher = createAllowedChatSenderMatcher({ + normalizeSender, + parseAllowTarget, + allowConversationTargets: true, + }); + + for (const entry of ["chat_id:123", "chat_guid:thread-123", "chat_identifier:team"]) { + expect( + matcher({ + allowFrom: [entry], + sender: "other@example.com", + chatId: 123, + chatGuid: "thread-123", + chatIdentifier: "team", + }), + ).toBe(true); + } + }); +}); diff --git a/src/channels/plugins/chat-target-prefixes.ts b/src/channels/plugins/chat-target-prefixes.ts index 8f2a4f42bea..79a12178778 100644 --- a/src/channels/plugins/chat-target-prefixes.ts +++ b/src/channels/plugins/chat-target-prefixes.ts @@ -27,6 +27,7 @@ export type ChatSenderAllowParams = { chatId?: number | null; chatGuid?: string | null; chatIdentifier?: string | null; + allowConversationTargets?: boolean | null; }; export function isAllowedParsedChatSender(params: { @@ -35,6 +36,7 @@ export function isAllowedParsedChatSender(params: { chatId?: number | null; chatGuid?: string | null; chatIdentifier?: string | null; + allowConversationTargets?: boolean | null; normalizeSender: (sender: string) => string; parseAllowTarget: (entry: string) => ParsedChatAllowTarget; }): boolean { @@ -47,9 +49,12 @@ export function isAllowedParsedChatSender(params: { } const senderNormalized = params.normalizeSender(params.sender); - const chatId = params.chatId ?? undefined; - const chatGuid = normalizeOptionalString(params.chatGuid); - const chatIdentifier = normalizeOptionalString(params.chatIdentifier); + const allowConversationTargets = params.allowConversationTargets === true; + const chatId = allowConversationTargets ? (params.chatId ?? undefined) : undefined; + const chatGuid = allowConversationTargets ? normalizeOptionalString(params.chatGuid) : undefined; + const chatIdentifier = allowConversationTargets + ? normalizeOptionalString(params.chatIdentifier) + : undefined; for (const entry of allowFrom) { if (!entry) { @@ -227,6 +232,7 @@ export function resolveServicePrefixedOrChatAllowTarget< export function createAllowedChatSenderMatcher(params: { normalizeSender: (sender: string) => string; parseAllowTarget: (entry: string) => ParsedChatAllowTarget; + allowConversationTargets?: boolean; }): (input: ChatSenderAllowParams) => boolean { return (input) => isAllowedParsedChatSender({ @@ -235,6 +241,8 @@ export function createAllowedChatSenderMatcher(params: { chatId: input.chatId, chatGuid: input.chatGuid, chatIdentifier: input.chatIdentifier, + allowConversationTargets: + input.allowConversationTargets ?? params.allowConversationTargets ?? false, normalizeSender: params.normalizeSender, parseAllowTarget: params.parseAllowTarget, }); diff --git a/src/plugin-sdk/allow-from.test.ts b/src/plugin-sdk/allow-from.test.ts index 0353534cb2f..cf772312e21 100644 --- a/src/plugin-sdk/allow-from.test.ts +++ b/src/plugin-sdk/allow-from.test.ts @@ -64,17 +64,67 @@ describe("isAllowedParsedChatSender", () => { expected: true, }, { - name: "matches chat IDs when provided", - input: { - allowFrom: ["chat_id:42"], - sender: "+15551234567", - chatId: 42, - normalizeSender: (sender: string) => sender, - parseAllowTarget, - }, - expected: true, + name: "does not match conversation targets by default", + input: [ + { + allowFrom: ["chat_id:42"], + sender: "+15551234567", + chatId: 42, + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + { + allowFrom: ["chat_guid:thread-42"], + sender: "+15551234567", + chatGuid: "thread-42", + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + { + allowFrom: ["chat_identifier:team"], + sender: "+15551234567", + chatIdentifier: "team", + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + ], + expected: [false, false, false], + }, + { + name: "matches conversation targets when they are enabled", + input: [ + { + allowFrom: ["chat_id:42"], + sender: "+15551234567", + chatId: 42, + allowConversationTargets: true, + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + { + allowFrom: ["chat_guid:thread-42"], + sender: "+15551234567", + chatGuid: "thread-42", + allowConversationTargets: true, + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + { + allowFrom: ["chat_identifier:team"], + sender: "+15551234567", + chatIdentifier: "team", + allowConversationTargets: true, + normalizeSender: (sender: string) => sender, + parseAllowTarget, + }, + ], + expected: [true, true, true], }, ])("$name", ({ input, expected }) => { + if (Array.isArray(input)) { + expect(input.map((entry) => isAllowedParsedChatSender(entry))).toEqual(expected); + return; + } expect(isAllowedParsedChatSender(input)).toBe(expected); }); }); diff --git a/src/plugin-sdk/allow-from.ts b/src/plugin-sdk/allow-from.ts index 492ee6df9dc..31a4c2ea38e 100644 --- a/src/plugin-sdk/allow-from.ts +++ b/src/plugin-sdk/allow-from.ts @@ -81,13 +81,14 @@ type ParsedChatAllowTarget = | { kind: "chat_identifier"; chatIdentifier: string } | { kind: "handle"; handle: string }; -/** Match chat-aware allowlist entries against sender, chat id, guid, or identifier fields. */ +/** Match allowlist entries against senders, with conversation targets requiring explicit opt-in. */ export function isAllowedParsedChatSender(params: { allowFrom: Array; sender: string; chatId?: number | null; chatGuid?: string | null; chatIdentifier?: string | null; + allowConversationTargets?: boolean | null; normalizeSender: (sender: string) => string; parseAllowTarget: (entry: string) => ParsedChatAllowTarget; }): boolean {