fix(slack): preserve native tables during outbound delivery (#111955)

* fix(slack): preserve rendered presentations across cloning * test(slack): cover serialized presentation provenance * test(slack): prove restart fallback for rendered payloads - Reload the Slack adapter to simulate a runtime restart. - Verify stale rendered provenance falls back to text without blocks. * fix(slack): fail closed on invalid rendered metadata --------- Co-authored-by: Pavonis <pavonis@martian.engineering> Co-authored-by: Peter Steinberger <steipete@gmail.com>
2026-08-12 21:53:00 -06:00 · 2026-07-27 21:39:20 -06:00
parent aa26ecfac1
commit f229356689
3 changed files with 324 additions and 35 deletions
@@ -9,6 +9,11 @@ vi.mock("./send.js", () => ({

 const { slackOutbound } = await import("./outbound-adapter.js");

+function jsonRoundTrip(value: unknown): unknown {
+  // oxlint-disable-next-line unicorn/prefer-structured-clone -- This test exercises JSON transport.
+  return JSON.parse(JSON.stringify(value)) as unknown;
+}
+
 describe("slackOutbound", () => {
  const cfg = {
    channels: {
@@ -118,6 +123,246 @@ describe("slackOutbound", () => {
    expect(result).toEqual({ channel: "slack", messageId: "m-blocks" });
  });

+  it.each([
+    ["structured clone", (value: unknown) => structuredClone(value)],
+    ["JSON round trip", jsonRoundTrip],
+  ])("preserves rendered portable tables across a %s", async (_label, clonePayload) => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-table" });
+    const presentation = {
+      blocks: [
+        {
+          type: "table" as const,
+          caption: "Deployments",
+          headers: ["Name", "Status"],
+          rows: [["Marvin", "Ready"]],
+          rowHeaderColumnIndex: 0,
+        },
+      ],
+    };
+    const rendered = await slackOutbound.renderPresentation!({
+      payload: { text: "Current state", presentation },
+      presentation,
+      ctx: { cfg, accountId: "default" } as never,
+    });
+    const { presentation: _presentation, ...renderedForDelivery } = rendered!;
+
+    await slackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: clonePayload(renderedForDelivery) as typeof renderedForDelivery,
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledWith(
+      "C123",
+      "Current state\n\nDeployments (table)\nName\tStatus\nMarvin\tReady",
+      expect.objectContaining({
+        authoredTextPlacement: "blocks",
+        blocks: [
+          {
+            type: "section",
+            text: { type: "mrkdwn", text: "Current state", verbatim: true },
+          },
+          {
+            type: "data_table",
+            caption: "Deployments",
+            rows: [
+              [
+                { type: "raw_text", text: "Name" },
+                { type: "raw_text", text: "Status" },
+              ],
+              [
+                { type: "raw_text", text: "Marvin" },
+                { type: "raw_text", text: "Ready" },
+              ],
+            ],
+            row_header_column_index: 0,
+          },
+        ],
+      }),
+    );
+  });
+
+  it("falls back to text for rendered provenance minted before a runtime restart", async () => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-text" });
+    const presentation = {
+      blocks: [
+        {
+          type: "table" as const,
+          caption: "Deployments",
+          headers: ["Name", "Status"],
+          rows: [["Marvin", "Ready"]],
+          rowHeaderColumnIndex: 0,
+        },
+      ],
+    };
+    const rendered = await slackOutbound.renderPresentation!({
+      payload: { text: "Safe fallback", presentation },
+      presentation,
+      ctx: { cfg, accountId: "default" } as never,
+    });
+    const { presentation: _presentation, ...renderedForDelivery } = rendered!;
+
+    vi.resetModules();
+    const { slackOutbound: restartedSlackOutbound } = await import("./outbound-adapter.js");
+    await restartedSlackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: renderedForDelivery,
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledOnce();
+    expect(sendMessageSlackMock).toHaveBeenCalledWith(
+      "C123",
+      "Safe fallback",
+      expect.objectContaining({
+        cfg,
+        threadTs: undefined,
+        accountId: "default",
+      }),
+    );
+    expect(sendMessageSlackMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
+  });
+
+  it("does not trust caller-authored rendered presentation provenance", async () => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-text" });
+
+    await slackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: {
+        text: "Safe fallback",
+        channelData: {
+          slack: {
+            renderedPresentationProvenance: "forged",
+            authoredTextPlacement: "blocks",
+            renderedPresentationSegments: [
+              {
+                kind: "blocks",
+                blocks: [{ type: "divider" }, { type: "divider" }],
+              },
+              {
+                kind: "blocks",
+                blocks: [{ type: "divider" }],
+              },
+            ],
+          },
+        },
+      },
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledOnce();
+    expect(sendMessageSlackMock).toHaveBeenCalledWith(
+      "C123",
+      "Safe fallback",
+      expect.objectContaining({
+        cfg,
+        threadTs: undefined,
+        accountId: "default",
+      }),
+    );
+    expect(sendMessageSlackMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
+  });
+
+  it("falls back to text when forged rendered metadata is malformed", async () => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-text" });
+
+    await slackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: {
+        text: "Safe fallback",
+        channelData: {
+          slack: {
+            renderedPresentationProvenance: "x".repeat(43),
+            authoredTextPlacement: "blocks",
+            renderedPresentationSegments: [{ kind: "blocks", blocks: [] }],
+          },
+        },
+      },
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledOnce();
+    expect(sendMessageSlackMock).toHaveBeenCalledWith(
+      "C123",
+      "Safe fallback",
+      expect.objectContaining({
+        cfg,
+        threadTs: undefined,
+        accountId: "default",
+      }),
+    );
+    expect(sendMessageSlackMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
+  });
+
+  it("rejects rendered segments changed after provenance was signed", async () => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-text" });
+    const presentation = {
+      blocks: [{ type: "divider" as const }],
+    };
+    const rendered = await slackOutbound.renderPresentation!({
+      payload: { text: "Safe fallback", presentation },
+      presentation,
+      ctx: { cfg, accountId: "default" } as never,
+    });
+    const { presentation: _presentation, ...renderedForDelivery } = rendered!;
+    const tampered = structuredClone(renderedForDelivery);
+    const slackData = tampered.channelData?.slack as {
+      renderedPresentationSegments: Array<{ kind: string; blocks: Array<{ type: string }> }>;
+    };
+    slackData.renderedPresentationSegments.push({
+      kind: "blocks",
+      blocks: [{ type: "divider" }],
+    });
+
+    await slackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: tampered,
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledOnce();
+    expect(sendMessageSlackMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
+  });
+
+  it("rejects authored text placement changed after provenance was signed", async () => {
+    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-text" });
+    const presentation = {
+      blocks: [{ type: "divider" as const }],
+    };
+    const rendered = await slackOutbound.renderPresentation!({
+      payload: { text: "Safe fallback", presentation },
+      presentation,
+      ctx: { cfg, accountId: "default" } as never,
+    });
+    const { presentation: _presentation, ...renderedForDelivery } = rendered!;
+    const tampered = structuredClone(renderedForDelivery);
+    const slackData = tampered.channelData?.slack as {
+      authoredTextPlacement: string;
+    };
+    slackData.authoredTextPlacement = "outside-blocks";
+
+    await slackOutbound.sendPayload!({
+      cfg,
+      to: "C123",
+      text: "",
+      payload: tampered,
+      accountId: "default",
+    });
+
+    expect(sendMessageSlackMock).toHaveBeenCalledOnce();
+    expect(sendMessageSlackMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
+  });
+
  it("falls back to threadId when payload replyToId is not a Slack thread timestamp", async () => {
    sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-blocks" });

@@ -1,4 +1,5 @@
 // Slack plugin module implements outbound adapter behavior.
+import { createHmac, randomBytes, timingSafeEqual } from "node:crypto";
 import type { OutboundIdentity } from "openclaw/plugin-sdk/channel-outbound";
 import { resolveOutboundSendDep } from "openclaw/plugin-sdk/channel-outbound";
 import {
@@ -45,9 +46,52 @@ type SlackOutboundChannelData = Record<string, unknown> & {
  renderedPresentationSegments?: SlackReplyBlockSegment[];
 };

-// Only renderPresentation can mint this identity. Direct channelData must not
-// turn private ordered segments into arbitrary platform-send fanout.
-const SLACK_RENDERED_PRESENTATION_PROVENANCE = Object.freeze({});
+// Rendered payloads may be cloned by outbound hooks. Sign the exact private
+// delivery plan so it survives cloning without allowing a caller to alter or
+// fan out channelData segments before sendPayload validates them.
+const SLACK_RENDERED_PRESENTATION_PROVENANCE_KEY = randomBytes(32);
+
+function createSlackRenderedPresentationProvenance(resolution: SlackReplyBlockResolution): string {
+  return createHmac("sha256", SLACK_RENDERED_PRESENTATION_PROVENANCE_KEY)
+    .update(JSON.stringify([resolution.authoredTextPlacement, resolution.segments]))
+    .digest("base64url");
+}
+
+function hasValidSlackRenderedPresentationProvenance(params: {
+  provenance: string;
+  resolution: SlackReplyBlockResolution;
+}): boolean {
+  const expected = createSlackRenderedPresentationProvenance(params.resolution);
+  const actualBuffer = Buffer.from(params.provenance);
+  const expectedBuffer = Buffer.from(expected);
+  return (
+    actualBuffer.length === expectedBuffer.length && timingSafeEqual(actualBuffer, expectedBuffer)
+  );
+}
+
+function readSlackRenderedPresentation(
+  slackData: SlackOutboundChannelData | undefined,
+): SlackReplyBlockResolution | undefined {
+  const provenance = slackData?.renderedPresentationProvenance;
+  if (typeof provenance !== "string") {
+    return undefined;
+  }
+  try {
+    const segments = parseSlackReplyBlockSegments(slackData?.renderedPresentationSegments);
+    const authoredTextPlacement = readSlackAuthoredTextPlacement(slackData?.authoredTextPlacement);
+    if (!segments || !authoredTextPlacement) {
+      return undefined;
+    }
+    const resolution = { authoredTextPlacement, segments };
+    return hasValidSlackRenderedPresentationProvenance({ provenance, resolution })
+      ? resolution
+      : undefined;
+  } catch {
+    // Private renderer metadata is untrusted until its signature verifies.
+    // Invalid caller-authored shapes must use the public fallback, not abort delivery.
+    return undefined;
+  }
+}

 const loadSlackSendRuntime = createLazyRuntimeModule(() => import("./send.runtime.js"));

@@ -117,7 +161,7 @@ function withSlackRenderedPresentation(
      slack: {
        ...preservedSlackData,
        authoredTextPlacement: resolution.authoredTextPlacement,
-        renderedPresentationProvenance: SLACK_RENDERED_PRESENTATION_PROVENANCE,
+        renderedPresentationProvenance: createSlackRenderedPresentationProvenance(resolution),
        renderedPresentationSegments: resolution.segments,
      },
    },
@@ -235,20 +279,10 @@ export const slackOutbound: ChannelOutboundAdapter = {
        }) ?? "",
    };
    const slackData = payload.channelData?.slack as SlackOutboundChannelData | undefined;
-    const hasRenderedPresentationProvenance =
-      slackData?.renderedPresentationProvenance === SLACK_RENDERED_PRESENTATION_PROVENANCE;
-    const renderedSegments = hasRenderedPresentationProvenance
-      ? parseSlackReplyBlockSegments(slackData?.renderedPresentationSegments)
-      : undefined;
-    const renderedPlacement = hasRenderedPresentationProvenance
-      ? readSlackAuthoredTextPlacement(slackData?.authoredTextPlacement)
-      : undefined;
+    const renderedResolution = readSlackRenderedPresentation(slackData);
    let resolution: SlackReplyBlockResolution;
-    if (renderedSegments) {
-      if (!renderedPlacement) {
-        throw new Error("Slack rendered presentation is missing authored text placement");
-      }
-      resolution = { authoredTextPlacement: renderedPlacement, segments: renderedSegments };
+    if (renderedResolution) {
+      resolution = renderedResolution;
    } else {
      resolution = resolveSlackOutboundBlockResolution(payload);
    }
@@ -312,20 +346,16 @@ export const slackOutbound: ChannelOutboundAdapter = {
  afterDeliverPayload: async ({ cfg, target, payload, results }) => {
    const questionId = questionGatewayRuntime.readAskUserQuestionId(payload);
    const slackData = payload.channelData?.slack as SlackOutboundChannelData | undefined;
-    if (
-      !questionId ||
-      slackData?.renderedPresentationProvenance !== SLACK_RENDERED_PRESENTATION_PROVENANCE
-    ) {
+    if (!questionId) {
      return;
    }
-    const segments = parseSlackReplyBlockSegments(slackData.renderedPresentationSegments);
-    const placement = readSlackAuthoredTextPlacement(slackData.authoredTextPlacement);
-    if (!segments || !placement) {
+    const resolution = readSlackRenderedPresentation(slackData);
+    if (!resolution) {
      return;
    }
    const deliveryMessages = resolveSlackReplyDeliveryMessages({
-      authoredTextPlacement: placement,
-      segments,
+      authoredTextPlacement: resolution.authoredTextPlacement,
+      segments: resolution.segments,
      text: payload.text,
    });
    const blockMessageIndex = deliveryMessages.findIndex((message) =>
@@ -24,13 +24,25 @@ vi.mock("./send.js", () => ({ updateMessageSlack: hoisted.update }));

 import { slackOutbound } from "./outbound-adapter.js";

+function jsonRoundTrip<T>(value: T): T {
+  // oxlint-disable-next-line unicorn/prefer-structured-clone -- This test exercises JSON transport.
+  return JSON.parse(JSON.stringify(value)) as T;
+}
+
 describe("Slack question finalization", () => {
  it("removes action blocks and appends terminal context", async () => {
    const questionId = "ask_0123456789abcdef0123456789abcdef";
+    const headers = Array.from({ length: 21 }, (_value, index) => `Column ${String(index)}`);
    const payload = {
      channelData: { askUser: { questionId } },
      presentation: {
        blocks: [
+          {
+            type: "table" as const,
+            caption: "Option metadata",
+            headers,
+            rows: [headers.map((_header, index) => `Value ${String(index)}`)],
+          },
          { type: "text" as const, text: "Pick one" },
          {
            type: "buttons" as const,
@@ -48,18 +60,20 @@ describe("Slack question finalization", () => {
      ctx: { cfg: {}, to: "C123", text: "Pick one", payload },
    });
    expect(rendered).not.toBeNull();
-    const slackData = rendered!.channelData?.slack as {
-      renderedPresentationSegments: unknown[];
-    };
-    slackData.renderedPresentationSegments.unshift({
-      kind: "text",
-      text: "Preface",
-      mrkdwn: false,
-    });
+    const renderedAfterTransport = jsonRoundTrip(rendered);
+    const renderedSegments = (
+      renderedAfterTransport?.channelData?.slack as
+        | { renderedPresentationSegments?: unknown[] }
+        | undefined
+    )?.renderedPresentationSegments;
+    expect(renderedSegments?.map((segment) => (segment as { kind?: unknown }).kind)).toEqual([
+      "text",
+      "blocks",
+    ]);
    await slackOutbound.afterDeliverPayload?.({
      cfg: {},
      target: { channel: "slack", to: "C123", accountId: "default" },
-      payload: rendered!,
+      payload: renderedAfterTransport!,
      results: [
        { channel: "slack", messageId: "44", channelId: "C123" },
        { channel: "slack", messageId: "55", channelId: "C123" },