From af9881b9c58cee242fda9d9b5fc26ad4524843eb Mon Sep 17 00:00:00 2001 From: Luis Conde <96428056+luijoc@users.noreply.github.com> Date: Sun, 22 Feb 2026 14:04:53 -0400 Subject: [PATCH] fix(slack): resolve user IDs to DM channels before files.uploadV2 (#23773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a bare Slack user ID (U-prefix) is passed as the send target without an explicit `user:` prefix, `parseSlackTarget` classifies it as kind="channel". `resolveChannelId` then passes it through to callers without calling `conversations.open`. This works for `chat.postMessage` (which tolerates user IDs), but `files.uploadV2` delegates to `completeUploadExternal` which validates `channel_id` against `^[CGDZ][A-Z0-9]{8,}$` — rejecting U-prefixed IDs with `invalid_arguments`. Fix: detect U-prefixed IDs in `resolveChannelId` regardless of the parsed `kind`, and always resolve them via `conversations.open` to obtain the DM channel ID (D-prefix). Includes test coverage for bare, prefixed, and mention-style user ID targets with file uploads, plus a channel-target negative case. --- CHANGELOG.md | 1 + src/slack/send.ts | 9 ++- src/slack/send.upload.test.ts | 123 ++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 src/slack/send.upload.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f8a450fc1..c2575beb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Docs: https://docs.openclaw.ai - Memory/Embeddings: apply configured remote-base host pinning (`allowedHostnames`) across OpenAI/Voyage/Gemini embedding requests to keep private/self-hosted endpoints working without cross-host drift. (#18198) Thanks @ianpcook. - Memory/Batch: route OpenAI/Voyage/Gemini batch upload/create/status/download requests through the same guarded HTTP path for consistent SSRF policy enforcement. - Install/Discord Voice: make `@discordjs/opus` an optional dependency so `openclaw` install/update no longer hard-fails when native Opus builds fail, while keeping `opusscript` as the runtime fallback decoder for Discord voice flows. (#23737, #23733, #23703) +- Slack/Upload: resolve bare user IDs (U-prefix) to DM channel IDs via `conversations.open` before calling `files.uploadV2`, which rejects non-channel IDs. `chat.postMessage` tolerates user IDs directly, but `files.uploadV2` → `completeUploadExternal` validates `channel_id` against `^[CGDZ][A-Z0-9]{8,}$`, causing `invalid_arguments` when agents reply with media to DM conversations. - Signal/RPC: guard malformed Signal RPC JSON responses with a clear status-scoped error and add regression coverage for invalid JSON responses. (#22995) Thanks @adhitShet. - Gateway/Subagents: guard gateway and subagent session-key/message trim paths against undefined inputs to prevent early `Cannot read properties of undefined (reading 'trim')` crashes during subagent spawn and wait flows. - Agents/Workspace: guard `resolveUserPath` against undefined/null input to prevent `Cannot read properties of undefined (reading 'trim')` crashes when workspace paths are missing in embedded runner flows. diff --git a/src/slack/send.ts b/src/slack/send.ts index d0b0f9c1a..590547397 100644 --- a/src/slack/send.ts +++ b/src/slack/send.ts @@ -166,7 +166,14 @@ async function resolveChannelId( client: WebClient, recipient: SlackRecipient, ): Promise<{ channelId: string; isDm?: boolean }> { - if (recipient.kind === "channel") { + // Bare Slack user IDs (U-prefix) may arrive with kind="channel" when the + // target string had no explicit prefix (parseSlackTarget defaults bare IDs + // to "channel"). chat.postMessage tolerates user IDs directly, but + // files.uploadV2 → completeUploadExternal validates channel_id against + // ^[CGDZ][A-Z0-9]{8,}$ and rejects U-prefixed IDs. Always resolve user + // IDs via conversations.open to obtain the DM channel ID. + const isUserId = recipient.kind === "user" || /^U[A-Z0-9]+$/i.test(recipient.id); + if (!isUserId) { return { channelId: recipient.id }; } const response = await client.conversations.open({ users: recipient.id }); diff --git a/src/slack/send.upload.test.ts b/src/slack/send.upload.test.ts new file mode 100644 index 000000000..1b534db64 --- /dev/null +++ b/src/slack/send.upload.test.ts @@ -0,0 +1,123 @@ +import type { WebClient } from "@slack/web-api"; +import { describe, expect, it, vi } from "vitest"; + +// --- Module mocks (must precede dynamic import) --- + +vi.mock("../config/config.js", () => ({ + loadConfig: () => ({}), +})); + +vi.mock("./accounts.js", () => ({ + resolveSlackAccount: () => ({ + accountId: "default", + botToken: "xoxb-test", + botTokenSource: "config", + config: {}, + }), +})); + +vi.mock("../web/media.js", () => ({ + loadWebMedia: vi.fn(async () => ({ + buffer: Buffer.from("fake-image"), + contentType: "image/png", + kind: "image", + fileName: "screenshot.png", + })), +})); + +const { sendMessageSlack } = await import("./send.js"); + +type UploadTestClient = WebClient & { + conversations: { open: ReturnType }; + chat: { postMessage: ReturnType }; + files: { uploadV2: ReturnType }; +}; + +function createUploadTestClient(): UploadTestClient { + return { + conversations: { + open: vi.fn(async () => ({ channel: { id: "D99RESOLVED" } })), + }, + chat: { + postMessage: vi.fn(async () => ({ ts: "171234.567" })), + }, + files: { + uploadV2: vi.fn(async () => ({ files: [{ id: "F001" }] })), + }, + } as unknown as UploadTestClient; +} + +describe("sendMessageSlack file upload with user IDs", () => { + it("resolves bare user ID to DM channel before files.uploadV2", async () => { + const client = createUploadTestClient(); + + // Bare user ID — parseSlackTarget classifies this as kind="channel" + await sendMessageSlack("U2ZH3MFSR", "screenshot", { + token: "xoxb-test", + client, + mediaUrl: "/tmp/screenshot.png", + }); + + // Should call conversations.open to resolve user ID → DM channel + expect(client.conversations.open).toHaveBeenCalledWith({ + users: "U2ZH3MFSR", + }); + + // files.uploadV2 should receive the resolved DM channel ID, not the user ID + expect(client.files.uploadV2).toHaveBeenCalledWith( + expect.objectContaining({ + channel_id: "D99RESOLVED", + filename: "screenshot.png", + }), + ); + }); + + it("resolves prefixed user ID to DM channel before files.uploadV2", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("user:UABC123", "image", { + token: "xoxb-test", + client, + mediaUrl: "/tmp/photo.png", + }); + + expect(client.conversations.open).toHaveBeenCalledWith({ + users: "UABC123", + }); + expect(client.files.uploadV2).toHaveBeenCalledWith( + expect.objectContaining({ channel_id: "D99RESOLVED" }), + ); + }); + + it("sends file directly to channel without conversations.open", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("channel:C123CHAN", "chart", { + token: "xoxb-test", + client, + mediaUrl: "/tmp/chart.png", + }); + + expect(client.conversations.open).not.toHaveBeenCalled(); + expect(client.files.uploadV2).toHaveBeenCalledWith( + expect.objectContaining({ channel_id: "C123CHAN" }), + ); + }); + + it("resolves mention-style user ID before file upload", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("<@U777TEST>", "report", { + token: "xoxb-test", + client, + mediaUrl: "/tmp/report.png", + }); + + expect(client.conversations.open).toHaveBeenCalledWith({ + users: "U777TEST", + }); + expect(client.files.uploadV2).toHaveBeenCalledWith( + expect.objectContaining({ channel_id: "D99RESOLVED" }), + ); + }); +});