fix(imessage): preserve service-prefixed direct handles as approvers

omarshahine · omarshahine · commit 008cf0e44409 · 2026-05-24T09:12:42.000-07:00
ClawSweeper P1 review finding on #85952. normalizeIMessageApproverId was calling looksLikeIMessageExplicitTargetId() to reject conversation-target prefixes, but that helper also matches the imessage:/sms:/auto: service prefixes — which are valid direct-handle forms. Any allowFrom entry like 'imessage:+15551230000' dropped to undefined, leaving approvers empty, which: - silently denied reaction resolution ('reactions require explicit approvers'), and - let text /approve fall back to implicit same-chat authorization. Fix: normalize first via normalizeIMessageHandle (strips the service prefix), then reject only chat_id:/chat_guid:/chat_identifier: conversation-target shapes that remain after normalization. Tests: - approval-auth.test.ts: assert the resolved approver list contains the normalized handle, plus the corollary that a non-matching sender is explicitly rejected (no longer masked by the implicit-same-chat fallback). Add a separate case covering chat_id/chat_guid/ chat_identifier rejection (with and without a service prefix). - approval-reactions.test.ts: reaction resolution end-to-end with a service-prefixed allowFrom entry — proves resolveIMessageApproval is called rather than silently denied. Focused suite: 48 passed (was 47).
diff --git a/extensions/imessage/src/approval-auth.test.ts b/extensions/imessage/src/approval-auth.test.ts
@@ -76,7 +76,31 @@ describe("imessageApprovalAuth", () => {
     ).toEqual({ authorized: true });
   });
 
-  it("strips imessage:/sms:/auto: service prefixes when matching senders", () => {
+  it("strips imessage:/sms:/auto: service prefixes when normalizing approver entries", () => {
+    // The resolved approver list itself must contain the bare normalized
+    // handle — a previous bug rejected service-prefixed entries entirely,
+    // which silently fell back to the empty-approvers implicit-same-chat
+    // authorization and masked the regression. Assert the explicit list here
+    // so reaction resolution (which requires a non-empty approver list) works
+    // for service-prefixed allowFrom values too.
+    expect(
+      getIMessageApprovalApprovers({
+        cfg: { channels: { imessage: { allowFrom: ["imessage:+15551230000"] } } },
+      }),
+    ).toEqual(["+15551230000"]);
+    expect(
+      getIMessageApprovalApprovers({
+        cfg: { channels: { imessage: { allowFrom: ["sms:+15551230001"] } } },
+      }),
+    ).toEqual(["+15551230001"]);
+    expect(
+      getIMessageApprovalApprovers({
+        cfg: { channels: { imessage: { allowFrom: ["auto:Owner@Example.com"] } } },
+      }),
+    ).toEqual(["owner@example.com"]);
+
+    // A sender that matches the normalized handle is explicitly authorized
+    // (not via the implicit same-chat fallback).
     expect(
       imessageApprovalAuth.authorizeActorAction({
         cfg: { channels: { imessage: { allowFrom: ["imessage:+15551230000"] } } },
@@ -85,5 +109,38 @@ describe("imessageApprovalAuth", () => {
         approvalKind: "exec",
       }),
     ).toEqual({ authorized: true });
+
+    // And a NON-matching sender is rejected — proving the entry was added
+    // to the approver list rather than collapsing to empty.
+    expect(
+      imessageApprovalAuth.authorizeActorAction({
+        cfg: { channels: { imessage: { allowFrom: ["imessage:+15551230000"] } } },
+        senderId: "+15559999999",
+        action: "approve",
+        approvalKind: "exec",
+      }),
+    ).toEqual({
+      authorized: false,
+      reason: "❌ You are not authorized to approve exec requests on iMessage.",
+    });
+  });
+
+  it("rejects chat_id / chat_guid / chat_identifier as approver entries even with service prefixes", () => {
+    expect(
+      getIMessageApprovalApprovers({
+        cfg: {
+          channels: {
+            imessage: {
+              allowFrom: [
+                "chat_id:42",
+                "chat_guid:iMessage;+;chat42",
+                "chat_identifier:chat42@example.com",
+                "imessage:chat_id:43",
+              ],
+            },
+          },
+        },
+      }),
+    ).toEqual([]);
   });
 });
diff --git a/extensions/imessage/src/approval-auth.ts b/extensions/imessage/src/approval-auth.ts
@@ -3,7 +3,7 @@ import {
   resolveApprovalApprovers,
 } from "openclaw/plugin-sdk/approval-auth-runtime";
 import { resolveIMessageAccount } from "./accounts.js";
-import { looksLikeIMessageExplicitTargetId, normalizeIMessageHandle } from "./targets.js";
+import { normalizeIMessageHandle } from "./targets.js";
 
 type ApprovalKind = "exec" | "plugin";
 
@@ -12,9 +12,12 @@ export function normalizeIMessageApproverId(value: string | number): string | un
   if (!raw) {
     return undefined;
   }
-  if (looksLikeIMessageExplicitTargetId(raw)) {
-    return undefined;
-  }
+  // Normalize first so service-prefixed direct handles (`imessage:+...`,
+  // `sms:+...`, `auto:+...`) are stripped to their bare identifier before we
+  // decide whether to reject the entry. After normalization only the
+  // conversation-target prefixes (chat_id / chat_guid / chat_identifier) remain
+  // as illegal approver shapes — service-prefixed direct handles are valid
+  // approver values that map to a specific phone/email.
   const normalized = normalizeIMessageHandle(raw);
   if (
     !normalized ||
diff --git a/extensions/imessage/src/approval-reactions.test.ts b/extensions/imessage/src/approval-reactions.test.ts
@@ -284,6 +284,44 @@ describe("iMessage approval reactions", () => {
     expect(resolverMocks.resolveIMessageApproval).toHaveBeenCalledTimes(1);
   });
 
+  it("resolves a reaction when the approver was configured with a service-prefixed allowFrom entry", async () => {
+    // Regression test for the ClawSweeper-flagged normalizer bug: a previous
+    // version of normalizeIMessageApproverId rejected service-prefixed direct
+    // handles (`imessage:+...`, `sms:+...`, `auto:+...`) before stripping the
+    // prefix, so the approver list collapsed to empty and reaction resolution
+    // silently denied with "reactions require explicit approvers".
+    registerIMessageApprovalReactionTarget({
+      accountId: "default",
+      conversation: { handle: "+15551230000" },
+      messageId: "approval-message",
+      approvalId: "exec-service-prefix",
+      allowedDecisions: ["allow-once", "deny"],
+    });
+
+    const cfg = {
+      channels: { imessage: { allowFrom: ["imessage:+15551230000"] } },
+    };
+    const handled = await maybeResolveIMessageApprovalReaction({
+      cfg,
+      accountId: "default",
+      message: buildTapbackReactionPayload({
+        sender: "+15551230000",
+        reaction_emoji: "👍",
+        reacted_to_guid: "approval-message",
+      }),
+      bodyText: "",
+    });
+
+    expect(handled).toBe(true);
+    expect(resolverMocks.resolveIMessageApproval).toHaveBeenCalledWith({
+      cfg,
+      approvalId: "exec-service-prefix",
+      decision: "allow-once",
+      senderId: "+15551230000",
+      gatewayUrl: undefined,
+    });
+  });
+
   it("resolves DM reactions even when send registered under handle but inbound carries chat_guid", async () => {
     registerIMessageApprovalReactionTarget({
       accountId: "default",
