diff --git a/extensions/slack/src/outbound-adapter.test.ts b/extensions/slack/src/outbound-adapter.test.ts index d8916c0979e8..8e04732943c6 100644 --- a/extensions/slack/src/outbound-adapter.test.ts +++ b/extensions/slack/src/outbound-adapter.test.ts @@ -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" }); diff --git a/extensions/slack/src/outbound-adapter.ts b/extensions/slack/src/outbound-adapter.ts index 1481514d02f1..d20cb247cf9f 100644 --- a/extensions/slack/src/outbound-adapter.ts +++ b/extensions/slack/src/outbound-adapter.ts @@ -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 & { 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) => diff --git a/extensions/slack/src/question-finalization.test.ts b/extensions/slack/src/question-finalization.test.ts index 65532b9dfc6d..3cd3472b1d4d 100644 --- a/extensions/slack/src/question-finalization.test.ts +++ b/extensions/slack/src/question-finalization.test.ts @@ -24,13 +24,25 @@ vi.mock("./send.js", () => ({ updateMessageSlack: hoisted.update })); import { slackOutbound } from "./outbound-adapter.js"; +function jsonRoundTrip(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" },