From 2a409bbba0cde6687ef489ef75dbd719a53a1057 Mon Sep 17 00:00:00 2001 From: Oleksandr Zakotyanskyi Date: Sun, 1 Mar 2026 18:37:18 +0100 Subject: [PATCH] fix(slack): replace files.uploadV2 with 3-step upload flow to fix missing_scope error (#17558) * fix(slack): replace files.uploadV2 with 3-step upload flow files.uploadV2 from @slack/web-api internally calls the deprecated files.upload endpoint, which fails with missing_scope even when files:write is correctly granted in the bot token scopes. Replace with Slack's recommended 3-step upload flow: 1. files.getUploadURLExternal - get presigned URL + file_id 2. fetch(upload_url) - upload file content 3. files.completeUploadExternal - finalize & share to channel/thread This preserves all existing behavior including thread replies via thread_ts and caption via initial_comment. * fix(slack): harden external upload flow and tests --------- Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/slack/send.ts | 80 +++++++++++++++++++------------ src/slack/send.upload.test.ts | 90 ++++++++++++++++++++++++++++++----- 3 files changed, 129 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68bc9cd52b7..bb71ad22b8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -530,7 +530,7 @@ Docs: https://docs.openclaw.ai - Slack/Threading: sessions: keep parent-session forking and thread-history context active beyond first turn by removing first-turn-only gates in session init, thread-history fetch, and reply prompt context injection. (#23843, #23090) Thanks @vincentkoc and @Taskle. - Slack/Threading: respect `replyToMode` when Slack auto-populates top-level `thread_ts`, and ignore inline `replyToId` directive tags when `replyToMode` is `off` so thread forcing stays disabled unless explicitly configured. (#23839, #23320, #23513) Thanks @vincentkoc and @dorukardahan. - Slack/Extension: forward `message read` `threadId` to `readMessages` and use delivery-context `threadId` as outbound `thread_ts` fallback so extension replies/reads stay in the correct Slack thread. (#22216, #22485, #23836) Thanks @vincentkoc, @lan17 and @dorukardahan. -- 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. +- Slack/Upload: resolve bare user IDs (U-prefix) to DM channel IDs via `conversations.open`, and replace `files.uploadV2` with Slack’s external 3-step upload flow (`files.getUploadURLExternal` → presigned upload POST → `files.completeUploadExternal`) to avoid `missing_scope`/`invalid_arguments` upload failures in DM and threaded media replies. - Webchat/Chat: apply assistant `final` payload messages directly to chat state so sent turns render without waiting for a full history refresh cycle. (#14928) Thanks @BradGroux. - Webchat/Chat: for out-of-band final events (for example tool-call side runs), append provided final assistant payloads directly instead of forcing a transient history reset. (#11139) Thanks @AkshayNavle. - Webchat/Performance: reload `chat.history` after final events only when the final payload lacks a renderable assistant message, avoiding expensive full-history refreshes on normal turns. (#20588) Thanks @amzzzzzzz. diff --git a/src/slack/send.ts b/src/slack/send.ts index ede97bafd71..7b42822960d 100644 --- a/src/slack/send.ts +++ b/src/slack/send.ts @@ -1,9 +1,4 @@ -import { - type Block, - type FilesUploadV2Arguments, - type KnownBlock, - type WebClient, -} from "@slack/web-api"; +import { type Block, type KnownBlock, type WebClient } from "@slack/web-api"; import { chunkMarkdownTextWithMode, resolveChunkMode, @@ -13,6 +8,7 @@ import { isSilentReplyText } from "../auto-reply/tokens.js"; import { loadConfig } from "../config/config.js"; import { resolveMarkdownTableMode } from "../config/markdown-tables.js"; import { logVerbose } from "../globals.js"; +import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; import { loadWebMedia } from "../web/media.js"; import type { SlackTokenSource } from "./accounts.js"; import { resolveSlackAccount } from "./accounts.js"; @@ -24,6 +20,10 @@ import { parseSlackTarget } from "./targets.js"; import { resolveSlackBotToken } from "./token.js"; const SLACK_TEXT_LIMIT = 4000; +const SLACK_UPLOAD_SSRF_POLICY = { + allowedHostnames: ["*.slack.com", "*.slack-edge.com", "*.slack-files.com"], + allowRfc2544BenchmarkRange: true, +}; type SlackRecipient = | { @@ -194,36 +194,54 @@ async function uploadSlackFile(params: { threadTs?: string; maxBytes?: number; }): Promise { - const { - buffer, - contentType: _contentType, - fileName, - } = await loadWebMedia(params.mediaUrl, { + const { buffer, contentType, fileName } = await loadWebMedia(params.mediaUrl, { maxBytes: params.maxBytes, localRoots: params.mediaLocalRoots, }); - const basePayload = { + // Use the 3-step upload flow (getUploadURLExternal -> POST -> completeUploadExternal) + // instead of files.uploadV2 which relies on the deprecated files.upload endpoint + // and can fail with missing_scope even when files:write is granted. + const uploadUrlResp = await params.client.files.getUploadURLExternal({ + filename: fileName ?? "upload", + length: buffer.length, + }); + if (!uploadUrlResp.ok || !uploadUrlResp.upload_url || !uploadUrlResp.file_id) { + throw new Error(`Failed to get upload URL: ${uploadUrlResp.error ?? "unknown error"}`); + } + + // Upload the file content to the presigned URL + const uploadBody = new Uint8Array(buffer) as BodyInit; + const { response: uploadResp, release } = await fetchWithSsrFGuard({ + url: uploadUrlResp.upload_url, + init: { + method: "POST", + ...(contentType ? { headers: { "Content-Type": contentType } } : {}), + body: uploadBody, + }, + policy: SLACK_UPLOAD_SSRF_POLICY, + proxy: "env", + auditContext: "slack-upload-file", + }); + try { + if (!uploadResp.ok) { + throw new Error(`Failed to upload file: HTTP ${uploadResp.status}`); + } + } finally { + await release(); + } + + // Complete the upload and share to channel/thread + const completeResp = await params.client.files.completeUploadExternal({ + files: [{ id: uploadUrlResp.file_id, title: fileName ?? "upload" }], channel_id: params.channelId, - file: buffer, - filename: fileName, ...(params.caption ? { initial_comment: params.caption } : {}), - // Note: filetype is deprecated in files.uploadV2, Slack auto-detects from file content - }; - const payload: FilesUploadV2Arguments = params.threadTs - ? { ...basePayload, thread_ts: params.threadTs } - : basePayload; - const response = await params.client.files.uploadV2(payload); - const parsed = response as { - files?: Array<{ id?: string; name?: string }>; - file?: { id?: string; name?: string }; - }; - const fileId = - parsed.files?.[0]?.id ?? - parsed.file?.id ?? - parsed.files?.[0]?.name ?? - parsed.file?.name ?? - "unknown"; - return fileId; + ...(params.threadTs ? { thread_ts: params.threadTs } : {}), + }); + if (!completeResp.ok) { + throw new Error(`Failed to complete upload: ${completeResp.error ?? "unknown error"}`); + } + + return uploadUrlResp.file_id; } export async function sendMessageSlack( diff --git a/src/slack/send.upload.test.ts b/src/slack/send.upload.test.ts index bc5f5c08ff7..c1adf325237 100644 --- a/src/slack/send.upload.test.ts +++ b/src/slack/send.upload.test.ts @@ -1,9 +1,22 @@ import type { WebClient } from "@slack/web-api"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { installSlackBlockTestMocks } from "./blocks.test-helpers.js"; // --- Module mocks (must precede dynamic import) --- installSlackBlockTestMocks(); +const fetchWithSsrFGuard = vi.fn( + async (params: { url: string; init?: RequestInit }) => + ({ + response: await fetch(params.url, params.init), + finalUrl: params.url, + release: async () => {}, + }) as const, +); + +vi.mock("../infra/net/fetch-guard.js", () => ({ + fetchWithSsrFGuard: (...args: unknown[]) => + fetchWithSsrFGuard(...(args as [params: { url: string; init?: RequestInit }])), +})); vi.mock("../web/media.js", () => ({ loadWebMedia: vi.fn(async () => ({ @@ -19,7 +32,10 @@ const { sendMessageSlack } = await import("./send.js"); type UploadTestClient = WebClient & { conversations: { open: ReturnType }; chat: { postMessage: ReturnType }; - files: { uploadV2: ReturnType }; + files: { + getUploadURLExternal: ReturnType; + completeUploadExternal: ReturnType; + }; }; function createUploadTestClient(): UploadTestClient { @@ -31,13 +47,30 @@ function createUploadTestClient(): UploadTestClient { postMessage: vi.fn(async () => ({ ts: "171234.567" })), }, files: { - uploadV2: vi.fn(async () => ({ files: [{ id: "F001" }] })), + getUploadURLExternal: vi.fn(async () => ({ + ok: true, + upload_url: "https://uploads.slack.test/upload", + file_id: "F001", + })), + completeUploadExternal: vi.fn(async () => ({ ok: true })), }, } as unknown as UploadTestClient; } describe("sendMessageSlack file upload with user IDs", () => { - it("resolves bare user ID to DM channel before files.uploadV2", async () => { + const originalFetch = globalThis.fetch; + + beforeEach(() => { + globalThis.fetch = vi.fn(async () => new Response("ok", { status: 200 })) as typeof fetch; + fetchWithSsrFGuard.mockClear(); + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + vi.restoreAllMocks(); + }); + + it("resolves bare user ID to DM channel before completing upload", async () => { const client = createUploadTestClient(); // Bare user ID — parseSlackTarget classifies this as kind="channel" @@ -52,16 +85,15 @@ describe("sendMessageSlack file upload with user IDs", () => { users: "U2ZH3MFSR", }); - // files.uploadV2 should receive the resolved DM channel ID, not the user ID - expect(client.files.uploadV2).toHaveBeenCalledWith( + expect(client.files.completeUploadExternal).toHaveBeenCalledWith( expect.objectContaining({ channel_id: "D99RESOLVED", - filename: "screenshot.png", + files: [expect.objectContaining({ id: "F001", title: "screenshot.png" })], }), ); }); - it("resolves prefixed user ID to DM channel before files.uploadV2", async () => { + it("resolves prefixed user ID to DM channel before completing upload", async () => { const client = createUploadTestClient(); await sendMessageSlack("user:UABC123", "image", { @@ -73,7 +105,7 @@ describe("sendMessageSlack file upload with user IDs", () => { expect(client.conversations.open).toHaveBeenCalledWith({ users: "UABC123", }); - expect(client.files.uploadV2).toHaveBeenCalledWith( + expect(client.files.completeUploadExternal).toHaveBeenCalledWith( expect.objectContaining({ channel_id: "D99RESOLVED" }), ); }); @@ -88,7 +120,7 @@ describe("sendMessageSlack file upload with user IDs", () => { }); expect(client.conversations.open).not.toHaveBeenCalled(); - expect(client.files.uploadV2).toHaveBeenCalledWith( + expect(client.files.completeUploadExternal).toHaveBeenCalledWith( expect.objectContaining({ channel_id: "C123CHAN" }), ); }); @@ -105,8 +137,44 @@ describe("sendMessageSlack file upload with user IDs", () => { expect(client.conversations.open).toHaveBeenCalledWith({ users: "U777TEST", }); - expect(client.files.uploadV2).toHaveBeenCalledWith( + expect(client.files.completeUploadExternal).toHaveBeenCalledWith( expect.objectContaining({ channel_id: "D99RESOLVED" }), ); }); + + it("uploads bytes to the presigned URL and completes with thread+caption", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("channel:C123CHAN", "caption", { + token: "xoxb-test", + client, + mediaUrl: "/tmp/threaded.png", + threadTs: "171.222", + }); + + expect(client.files.getUploadURLExternal).toHaveBeenCalledWith({ + filename: "screenshot.png", + length: Buffer.from("fake-image").length, + }); + expect(globalThis.fetch).toHaveBeenCalledWith( + "https://uploads.slack.test/upload", + expect.objectContaining({ + method: "POST", + }), + ); + expect(fetchWithSsrFGuard).toHaveBeenCalledWith( + expect.objectContaining({ + url: "https://uploads.slack.test/upload", + proxy: "env", + auditContext: "slack-upload-file", + }), + ); + expect(client.files.completeUploadExternal).toHaveBeenCalledWith( + expect.objectContaining({ + channel_id: "C123CHAN", + initial_comment: "caption", + thread_ts: "171.222", + }), + ); + }); });