mirror of https://github.com/openclaw/openclaw.git
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>
This commit is contained in:
Oleksandr Zakotyanskyi
GitHub
parent
39a45121d9
commit
2a409bbba0
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<string> {
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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<typeof vi.fn> };
|
||||
chat: { postMessage: ReturnType<typeof vi.fn> };
|
||||
files: { uploadV2: ReturnType<typeof vi.fn> };
|
||||
files: {
|
||||
getUploadURLExternal: ReturnType<typeof vi.fn>;
|
||||
completeUploadExternal: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
};
|
||||
|
||||
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",
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue