From 245dc90543db2ea6837548a96a30fb5d4a6ee0db Mon Sep 17 00:00:00 2001 From: joshavant <830519+joshavant@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:30:30 -0500 Subject: [PATCH] fix(imessage): harden native approval polls --- .../imessage/src/actions.runtime.test.ts | 20 +- extensions/imessage/src/actions.runtime.ts | 9 + extensions/imessage/src/actions.test.ts | 3 + extensions/imessage/src/actions.ts | 12 +- .../src/approval-handler.runtime.test.ts | 5 +- .../imessage/src/approval-handler.runtime.ts | 12 +- .../imessage/src/approval-polls.test.ts | 19 ++ extensions/imessage/src/approval-polls.ts | 20 +- .../src/monitor.approval-poll-ingress.test.ts | 180 ++++++++++++++++++ .../imessage/src/monitor/monitor-provider.ts | 7 +- extensions/imessage/src/status.test.ts | 90 ++++++++- 11 files changed, 355 insertions(+), 22 deletions(-) create mode 100644 extensions/imessage/src/monitor.approval-poll-ingress.test.ts diff --git a/extensions/imessage/src/actions.runtime.test.ts b/extensions/imessage/src/actions.runtime.test.ts index 5bcf449161c3..e6a80a97497d 100644 --- a/extensions/imessage/src/actions.runtime.test.ts +++ b/extensions/imessage/src/actions.runtime.test.ts @@ -81,10 +81,15 @@ describe("imessage actions runtime", () => { it("suppresses the imsg poll caption when the caller already rendered context", async () => { runIMessageCliJsonCommandMock.mockResolvedValue({ guid: "poll-guid", - poll: { options: [] }, + poll: { + options: [ + { id: " option-allow ", text: "Allow" }, + { id: "option-deny", text: " Deny " }, + ], + }, }); - await imessageActionsRuntime.sendPoll({ + const result = await imessageActionsRuntime.sendPoll({ chatGuid: "iMessage;+;chat0000", question: "Approval details", choices: ["Allow", "Deny"], @@ -114,6 +119,13 @@ describe("imessage actions runtime", () => { "--no-comment", ], }); + expect(result).toEqual({ + messageId: "poll-guid", + pollOptions: [ + { id: "option-allow", text: "Allow" }, + { id: "option-deny", text: "Deny" }, + ], + }); }); it("drops cached chats.list entries when the current clock is not a valid date timestamp", async () => { @@ -125,12 +137,14 @@ describe("imessage actions runtime", () => { imessageActionsRuntime.resolveChatGuidForTarget({ target: { kind: "chat_id", chatId: 1 }, options: { cliPath: "imsg-invalid-clock" }, + conversationReadOrigin: "delegated", }), ).resolves.toBe("iMessage;+;first"); await expect( imessageActionsRuntime.resolveChatGuidForTarget({ target: { kind: "chat_id", chatId: 2 }, options: { cliPath: "imsg-invalid-clock" }, + conversationReadOrigin: "delegated", }), ).resolves.toBe("iMessage;+;second"); @@ -156,12 +170,14 @@ describe("imessage actions runtime", () => { imessageActionsRuntime.resolveChatGuidForTarget({ target: { kind: "chat_id", chatId: 1 }, options: { cliPath: "imsg-overflow-clock" }, + conversationReadOrigin: "direct-operator", }), ).resolves.toBe("iMessage;+;first"); await expect( imessageActionsRuntime.resolveChatGuidForTarget({ target: { kind: "chat_id", chatId: 2 }, options: { cliPath: "imsg-overflow-clock" }, + conversationReadOrigin: "direct-operator", }), ).resolves.toBe("iMessage;+;second"); diff --git a/extensions/imessage/src/actions.runtime.ts b/extensions/imessage/src/actions.runtime.ts index 3f9aca5f7aa7..10f44802e380 100644 --- a/extensions/imessage/src/actions.runtime.ts +++ b/extensions/imessage/src/actions.runtime.ts @@ -1,6 +1,7 @@ // Imessage plugin module implements actions behavior. import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { extname, join } from "node:path"; +import type { ChannelMessageActionContext } from "openclaw/plugin-sdk/channel-contract"; import { asDateTimestampMs, parseStrictInteger, @@ -32,6 +33,10 @@ type IMessageBridgeSendResult = { messageId: string; }; +type IMessageConversationReadOrigin = NonNullable< + ChannelMessageActionContext["conversationReadOrigin"] +>; + /** Option identity assigned by Messages when the poll balloon was created. */ export type IMessagePollSentOption = { id: string; @@ -250,7 +255,11 @@ export const imessageActionsRuntime = { async resolveChatGuidForTarget(params: { target: Extract; options: CliRunOptions; + conversationReadOrigin: IMessageConversationReadOrigin; }): Promise { + // Requiring the host-normalized origin at this list-backed read seam keeps + // direct operator lookups distinct from delegated actions, which have + // already passed the core exact-current-conversation gate. // Each `chats.list` call spawns a fresh imsg rpc subprocess and pulls // every chat the account knows about. Bursts of agent actions (react // then reply, reply then add-participant, etc.) all paid that cost diff --git a/extensions/imessage/src/actions.test.ts b/extensions/imessage/src/actions.test.ts index 2d6515ab8f72..183281e62677 100644 --- a/extensions/imessage/src/actions.test.ts +++ b/extensions/imessage/src/actions.test.ts @@ -984,6 +984,7 @@ describe("imessage message actions", () => { { target: { kind: "chat_id", chatId: 42 }, options: imsgOptions(), + conversationReadOrigin: "delegated", }, ], ]); @@ -1111,6 +1112,7 @@ describe("imessage message actions", () => { { target: { kind: "chat_identifier", chatIdentifier: "team-thread" }, options: imsgOptions(), + conversationReadOrigin: "delegated", }, ], ]); @@ -1355,6 +1357,7 @@ describe("imessage message actions", () => { chatIdentifier: "iMessage;-;+12069106512", }, options: imsgOptions(), + conversationReadOrigin: "direct-operator", }, ], ]); diff --git a/extensions/imessage/src/actions.ts b/extensions/imessage/src/actions.ts index 0d6d59b65b69..60dc99373ba7 100644 --- a/extensions/imessage/src/actions.ts +++ b/extensions/imessage/src/actions.ts @@ -11,6 +11,7 @@ import { } from "openclaw/plugin-sdk/channel-actions"; import type { ChannelMessageActionAdapter, + ChannelMessageActionContext, ChannelMessageActionName, } from "openclaw/plugin-sdk/channel-contract"; import { createLazyRuntimeNamedExport } from "openclaw/plugin-sdk/lazy-runtime"; @@ -55,6 +56,10 @@ const GROUP_MANAGEMENT_ACTIONS = new Set([ "leaveGroup", ]); +type IMessageConversationReadOrigin = NonNullable< + ChannelMessageActionContext["conversationReadOrigin"] +>; + function readMessageText(params: Record): string | undefined { return readStringParam(params, "text") ?? readStringParam(params, "message"); } @@ -169,6 +174,7 @@ async function resolveChatGuid(params: { action: ChannelMessageActionName; actionParams: Record; currentChannelId?: string; + conversationReadOrigin: IMessageConversationReadOrigin; runtime: IMessageActionsRuntime; options: { cliPath: string; @@ -185,6 +191,7 @@ async function resolveChatGuid(params: { const resolved = await params.runtime.resolveChatGuidForTarget({ target, options: params.options, + conversationReadOrigin: params.conversationReadOrigin, }); if (resolved) { return resolved; @@ -202,6 +209,7 @@ async function resolveChatGuid(params: { const resolved = await params.runtime.resolveChatGuidForTarget({ target: { kind: "chat_identifier", chatIdentifier: synthesizedIdentifier }, options: params.options, + conversationReadOrigin: params.conversationReadOrigin, }); if (resolved) { return resolved; @@ -491,11 +499,13 @@ export const imessageMessageActions: ChannelMessageActionAdapter = { timeoutMs: account.config.probeTimeoutMs, chatGuid: "", }; + const attestedConversationReadOrigin = conversationReadOrigin ?? "delegated"; const chatGuid = async () => await resolveChatGuid({ action, actionParams: params, currentChannelId: toolContext?.currentChannelId, + conversationReadOrigin: attestedConversationReadOrigin, runtime, options: opts, }); @@ -525,7 +535,7 @@ export const imessageMessageActions: ChannelMessageActionAdapter = { dbPath: opts.dbPath, }), remoteHost: opts.remoteHost, - conversationReadOrigin, + conversationReadOrigin: attestedConversationReadOrigin, }, }); }; diff --git a/extensions/imessage/src/approval-handler.runtime.test.ts b/extensions/imessage/src/approval-handler.runtime.test.ts index e56e6d3c73f5..c4cec36bbd7d 100644 --- a/extensions/imessage/src/approval-handler.runtime.test.ts +++ b/extensions/imessage/src/approval-handler.runtime.test.ts @@ -681,7 +681,10 @@ describe("imessageApprovalNativeRuntime", () => { }); expect(actionsMock.resolveChatGuidForTarget).toHaveBeenCalledWith( - expect.objectContaining({ target: { kind: "chat_id", chatId: 42 } }), + expect.objectContaining({ + target: { kind: "chat_id", chatId: 42 }, + conversationReadOrigin: "direct-operator", + }), ); expect(actionsMock.sendPoll).toHaveBeenCalled(); expect(entry).toMatchObject({ poll: expect.anything(), reactionFallbackVisible: true }); diff --git a/extensions/imessage/src/approval-handler.runtime.ts b/extensions/imessage/src/approval-handler.runtime.ts index 3aadb4d86061..3edea09241e6 100644 --- a/extensions/imessage/src/approval-handler.runtime.ts +++ b/extensions/imessage/src/approval-handler.runtime.ts @@ -208,10 +208,7 @@ function resolveIMessageApprovalCliOptions(params: { * * Conversation-read authority: `chatGuid` is resolved from the approval's own * routing target (origin session or a configured approver), so this read is - * host-originated, not delegated. The bridge runtime seam does not yet accept - * an attested `conversationReadOrigin` the way `sendMessageIMessage` does; the - * safety here rests on the target never being caller-supplied. Route this - * through the attested seam once it exists rather than widening the target. + * host-originated and carries the server-owned direct-operator attestation. * */ async function deliverIMessageApprovalPoll(params: { @@ -331,7 +328,11 @@ async function resolveIMessageApprovalChatGuid(params: { } const runtime = await loadIMessageActionsRuntime(); if (target.kind === "chat_id" || target.kind === "chat_identifier") { - return await runtime.resolveChatGuidForTarget({ target, options: params.cliOptions }); + return await runtime.resolveChatGuidForTarget({ + target, + options: params.cliOptions, + conversationReadOrigin: "direct-operator", + }); } if (target.kind !== "handle") { return null; @@ -340,6 +341,7 @@ async function resolveIMessageApprovalChatGuid(params: { return await runtime.resolveChatGuidForTarget({ target: { kind: "chat_identifier", chatIdentifier: `${service};-;${target.to}` }, options: params.cliOptions, + conversationReadOrigin: "direct-operator", }); } diff --git a/extensions/imessage/src/approval-polls.test.ts b/extensions/imessage/src/approval-polls.test.ts index 517c6c39af36..2ee56147c41b 100644 --- a/extensions/imessage/src/approval-polls.test.ts +++ b/extensions/imessage/src/approval-polls.test.ts @@ -292,6 +292,25 @@ describe("maybeResolveIMessageApprovalPollVote", () => { expect(resolverMocks.resolveIMessageApproval).not.toHaveBeenCalled(); }); + it("rejects imsg's local-identity sender fallback on received rows", async () => { + expect(bind()).toBe(true); + + await expect( + maybeResolveIMessageApprovalPollVote({ + cfg, + accountId: "default", + message: buildVote({ + sender: APPROVER, + participant: APPROVER, + isFromMe: false, + destinationCallerId: APPROVER, + }), + }), + ).resolves.toBe(false); + + expect(resolverMocks.resolveIMessageApproval).not.toHaveBeenCalled(); + }); + it("uses the option id when imsg reports the prompt GUID instead of the poll GUID", async () => { registerIMessageApprovalPollTarget({ accountId: "default", diff --git a/extensions/imessage/src/approval-polls.ts b/extensions/imessage/src/approval-polls.ts index ab0af5e4b846..5d9832caf8d0 100644 --- a/extensions/imessage/src/approval-polls.ts +++ b/extensions/imessage/src/approval-polls.ts @@ -305,15 +305,21 @@ function readPollVoteEvent(message: IMessagePayload): ApprovalPollVoteEvent | nu (typeof poll.poll_guid === "string" && poll.poll_guid) || "", ); - // chat.db authenticates received rows through sender. For a paired-device - // self-send, sender can be empty; destination_caller_id is the database's - // local-account identity and is accepted only when is_from_me is true. + // chat.db authenticates received rows through sender. Released imsg fills an + // empty sender from destination_caller_id before serialization, so reject a + // received row when those identities are equal: its remote actor is + // indistinguishable from the local-account fallback. Paired-device self-sends + // may use destination_caller_id only when is_from_me is authoritative. const sender = normalizeIMessageHandle((message.sender ?? "").trim()); + const destinationCallerId = normalizeIMessageHandle((message.destination_caller_id ?? "").trim()); + const receivedSenderIsLocalFallback = + message.is_from_me !== true && + Boolean(sender) && + Boolean(destinationCallerId) && + sender === destinationCallerId; const actorHandle = - sender || - (message.is_from_me === true - ? normalizeIMessageHandle((message.destination_caller_id ?? "").trim()) - : ""); + (receivedSenderIsLocalFallback ? "" : sender) || + (message.is_from_me === true ? destinationCallerId : ""); if (!pollGuid || !actorHandle) { return null; } diff --git a/extensions/imessage/src/monitor.approval-poll-ingress.test.ts b/extensions/imessage/src/monitor.approval-poll-ingress.test.ts new file mode 100644 index 000000000000..11eb30a3418e --- /dev/null +++ b/extensions/imessage/src/monitor.approval-poll-ingress.test.ts @@ -0,0 +1,180 @@ +// Imessage tests cover approval poll durable-ingress ownership. +import type { waitForTransportReady } from "openclaw/plugin-sdk/transport-ready-runtime"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { createIMessageRpcClient } from "./client.js"; +import { monitorIMessageProvider } from "./monitor.js"; +import { installIMessageStateRuntimeForTest } from "./test-support/runtime.js"; + +const waitForTransportReadyMock = vi.hoisted(() => + vi.fn(async () => {}), +); +const createIMessageRpcClientMock = vi.hoisted(() => vi.fn()); +const maybeResolveIMessageApprovalPollVoteMock = vi.hoisted(() => vi.fn()); +const ingressLifecycleMocks = vi.hoisted(() => ({ + onAdopted: vi.fn(async () => {}), + onDeferred: vi.fn(), + onAdoptionFinalizing: vi.fn(), + onAbandoned: vi.fn(async () => {}), +})); +const ingressHarness = vi.hoisted(() => ({ + done: Promise.resolve(), + resolveDone: () => {}, +})); + +vi.mock("openclaw/plugin-sdk/transport-ready-runtime", () => ({ + waitForTransportReady: waitForTransportReadyMock, +})); + +vi.mock("openclaw/plugin-sdk/channel-inbound", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createChannelInboundDebouncer: vi.fn( + (opts: { onFlush: (entries: unknown[]) => Promise }) => ({ + debouncer: { + enqueue: async (entry: unknown) => { + await opts.onFlush([entry]); + }, + }, + }), + ), + }; +}); + +vi.mock("./approval-polls.js", () => ({ + maybeResolveIMessageApprovalPollVote: maybeResolveIMessageApprovalPollVoteMock, +})); + +vi.mock("./approval-reaction-poller.js", () => ({ + pollPendingIMessageApprovalReactions: vi.fn(async () => {}), +})); + +vi.mock("./approval-reactions.js", () => ({ + maybeResolveIMessageApprovalReaction: vi.fn(async () => false), +})); + +vi.mock("./client.js", () => ({ + createIMessageRpcClient: createIMessageRpcClientMock, +})); + +vi.mock("./monitor/abort-handler.js", () => ({ + attachIMessageMonitorAbortHandler: vi.fn(() => () => {}), +})); + +vi.mock("./monitor/ingress.js", () => ({ + createIMessageDurableIngress: vi.fn( + (opts: { + dispatch: ( + message: unknown, + lifecycle: unknown, + receivedAt: number, + provenance: object, + ) => Promise; + }) => ({ + receive: async (raw: { message: unknown }) => { + try { + await opts.dispatch( + raw.message, + { + abortSignal: new AbortController().signal, + ...ingressLifecycleMocks, + }, + Date.now(), + {}, + ); + } finally { + ingressHarness.resolveDone(); + } + }, + start: vi.fn(), + stop: vi.fn(async () => {}), + }), + ), +})); + +function createRuntime() { + return { + log: vi.fn(), + error: vi.fn(), + }; +} + +describe("iMessage approval poll durable ingress", () => { + beforeEach(() => { + installIMessageStateRuntimeForTest(); + waitForTransportReadyMock.mockReset().mockResolvedValue(undefined); + createIMessageRpcClientMock.mockReset(); + maybeResolveIMessageApprovalPollVoteMock.mockReset(); + for (const mock of Object.values(ingressLifecycleMocks)) { + mock.mockClear(); + } + ingressHarness.done = new Promise((resolve) => { + ingressHarness.resolveDone = () => resolve(); + }); + }); + + function arrangeNotification() { + let onNotification: ((message: { method: string; params: unknown }) => void) | undefined; + createIMessageRpcClientMock.mockImplementation(async (params) => { + onNotification = params?.onNotification; + return { + request: vi.fn(async () => ({ subscription: 1 })), + waitForClose: vi.fn(async () => { + onNotification?.({ + method: "message", + params: { + message: { + id: 42, + guid: "approval-poll-vote-guid", + chat_id: 7, + chat_guid: "iMessage;-;redacted-peer", + chat_identifier: "redacted-peer", + sender: "redacted-peer", + is_from_me: false, + is_group: false, + created_at: new Date().toISOString(), + poll: { + kind: "vote", + original_guid: "approval-poll-guid", + vote: { option_id: "approve-once" }, + }, + }, + }, + }); + await ingressHarness.done; + }), + stop: vi.fn(async () => {}), + } as never; + }); + } + + it("completes a handled approval vote instead of abandoning it for replay", async () => { + arrangeNotification(); + maybeResolveIMessageApprovalPollVoteMock.mockResolvedValue(true); + + await monitorIMessageProvider({ + config: { channels: { imessage: {} } } as never, + runtime: createRuntime() as never, + }); + + expect(ingressLifecycleMocks.onAdopted).toHaveBeenCalledTimes(1); + expect(ingressLifecycleMocks.onAbandoned).not.toHaveBeenCalled(); + }); + + it("abandons a claim when approval vote resolution fails transiently", async () => { + arrangeNotification(); + maybeResolveIMessageApprovalPollVoteMock.mockRejectedValue(new Error("gateway unavailable")); + const runtime = createRuntime(); + + await monitorIMessageProvider({ + config: { channels: { imessage: {} } } as never, + runtime: runtime as never, + }); + + expect(ingressLifecycleMocks.onAdopted).not.toHaveBeenCalled(); + expect(ingressLifecycleMocks.onAbandoned).toHaveBeenCalledTimes(1); + expect(runtime.error).toHaveBeenCalledWith( + expect.stringContaining("imessage: inbound dispatch failed: Error: gateway unavailable"), + ); + }); +}); diff --git a/extensions/imessage/src/monitor/monitor-provider.ts b/extensions/imessage/src/monitor/monitor-provider.ts index 9734e8a81cf8..40380a30ae81 100644 --- a/extensions/imessage/src/monitor/monitor-provider.ts +++ b/extensions/imessage/src/monitor/monitor-provider.ts @@ -775,11 +775,8 @@ export async function monitorIMessageProvider(opts: MonitorIMessageOpts = {}): P message, }) ) { - // A vote is a real message row, so the debouncer still owns its ingress - // claim (see dispatch's `deferred` result). Resolving it here means no - // session will ever adopt it; abandon explicitly or the claim stalls - // until the 300s handler-timeout fires. - await ingressLifecycle?.onAbandoned(); + // Returning normally lets dispatchUnit settle the durable claim. Only a + // resolver error abandons it for replay; a handled vote must not retry. return; } diff --git a/extensions/imessage/src/status.test.ts b/extensions/imessage/src/status.test.ts index b4d03c4702a5..3178649a0ef9 100644 --- a/extensions/imessage/src/status.test.ts +++ b/extensions/imessage/src/status.test.ts @@ -720,7 +720,9 @@ describe("probeIMessage", () => { available: false, }); - expect(runCommand).toHaveBeenCalledTimes(4); + // Each uncached probe runs status plus both side-effect-free CLI capability + // checks (send-rich attachment and poll caption suppression). + expect(runCommand).toHaveBeenCalledTimes(6); }); it("propagates imsg's status message when advanced features are unavailable", async () => { @@ -756,6 +758,92 @@ describe("probeIMessage", () => { }); }); + it("detects poll caption suppression from the exact poll send help contract", async () => { + const runCommand = vi + .spyOn(processRuntime, "runCommandWithTimeout") + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + advanced_features: true, + v2_ready: true, + selectors: { pollPayloadMessage: true }, + rpc_methods: ["chats.list"], + }), + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }) + .mockResolvedValueOnce({ + stdout: "send-rich --file", + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }) + .mockResolvedValueOnce({ + stdout: "poll send --question --option --no-comment", + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }); + + await expect( + probeIMessagePrivateApi("imsg-poll-no-comment-supported", 1000), + ).resolves.toMatchObject({ + cliCapabilities: { pollSendSupportsNoComment: true }, + }); + expect(runCommand).toHaveBeenNthCalledWith( + 3, + ["imsg-poll-no-comment-supported", "poll", "send", "--help"], + { timeoutMs: 1000 }, + ); + }); + + it("does not infer poll caption suppression from selectors when the flag is absent", async () => { + vi.spyOn(processRuntime, "runCommandWithTimeout") + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + advanced_features: true, + v2_ready: true, + selectors: { pollPayloadMessage: true }, + rpc_methods: ["chats.list"], + }), + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }) + .mockResolvedValueOnce({ + stdout: "send-rich --file", + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }) + .mockResolvedValueOnce({ + stdout: "poll send --question --option ", + stderr: "", + code: 0, + signal: null, + killed: false, + termination: "exit", + }); + + await expect( + probeIMessagePrivateApi("imsg-poll-no-comment-absent", 1000), + ).resolves.toMatchObject({ + available: true, + selectors: { pollPayloadMessage: true }, + cliCapabilities: { pollSendSupportsNoComment: false }, + }); + }); + it("fails fast for default local imsg probes on non-mac hosts", async () => { const createIMessageRpcClientMock = vi .spyOn(clientModule, "createIMessageRpcClient")