Fix Slack file upload review issues

Author: ctate

apps/web/app/docs/configuration/page.mdx

@@ -216,6 +216,25 @@ slack:
       name: My Slack App
       redirect_uris:
         - http://localhost:3000/api/auth/callback/slack
+      scopes:
+        - chat:write
+        - channels:read
+        - users.profile:read
+        - users.profile:write
+        - users:write
+        - files:read
+        - files:write
+  tokens:
+    - token: xoxb-local-test
+      user: developer
+      scopes:
+        - chat:write
+        - channels:read
+        - users.profile:read
+        - users.profile:write
+        - users:write
+        - files:read
+        - files:write
   incoming_webhooks:
     - channel: general
       label: CI Notifications

packages/@emulators/slack/src/__tests__/slack.test.ts

@@ -3288,6 +3288,66 @@ describe("Slack plugin - files", () => {
     expect(ss.messages.all()).toHaveLength(0);
   });
 
+  it("does not create direct messages when file completion validation fails", async () => {
+    insertSlackTestUser(store, "U000000002", "file-dm-target");
+
+    const uploadRes = await app.request(`${base}/api/files.getUploadURLExternal`, {
+      method: "POST",
+      headers: authHeaders(),
+      body: JSON.stringify({ filename: "dm-failure.txt", length: 7 }),
+    });
+    const upload = (await uploadRes.json()) as any;
+    await app.request(upload.upload_url, { method: "POST", body: "failure" });
+
+    const completeRes = await app.request(`${base}/api/files.completeUploadExternal`, {
+      method: "POST",
+      headers: authHeaders(),
+      body: JSON.stringify({
+        files: [{ id: upload.file_id, title: "DM Failure" }],
+        channel_id: "U000000002",
+        blocks: "not-json",
+      }),
+    });
+    const completed = (await completeRes.json()) as any;
+    expect(completed.ok).toBe(false);
+    expect(completed.error).toBe("invalid_blocks");
+
+    const ss = getSlackStore(store);
+    expect(ss.channels.all().filter((channel) => channel.is_im)).toHaveLength(0);
+    expect(ss.files.findOneBy("file_id", upload.file_id)).toBeUndefined();
+    expect(ss.fileUploadSessions.findOneBy("file_id", upload.file_id)?.completed).toBe(false);
+  });
+
+  it("deduplicates resolved file share channels", async () => {
+    const channel = getSlackStore(store).channels.findOneBy("name", "general")!.channel_id;
+
+    const uploadRes = await app.request(`${base}/api/files.getUploadURLExternal`, {
+      method: "POST",
+      headers: authHeaders(),
+      body: JSON.stringify({ filename: "dedupe.txt", length: 6 }),
+    });
+    const upload = (await uploadRes.json()) as any;
+    await app.request(upload.upload_url, { method: "POST", body: "dedupe" });
+
+    const completeRes = await app.request(`${base}/api/files.completeUploadExternal`, {
+      method: "POST",
+      headers: authHeaders(),
+      body: JSON.stringify({
+        files: [{ id: upload.file_id, title: "Dedupe" }],
+        channel_id: channel,
+        channels: "general",
+      }),
+    });
+    const completed = (await completeRes.json()) as any;
+    expect(completed.ok).toBe(true);
+
+    const ss = getSlackStore(store);
+    const messages = ss.messages.findBy("channel_id", channel);
+    const file = ss.files.findOneBy("file_id", upload.file_id)!;
+    expect(messages).toHaveLength(1);
+    expect(file.shares.public?.[channel]).toHaveLength(1);
+  });
+
   it("enforces file scopes in strict mode", async () => {
     store.setData("slack.strict_scopes", true);
     tokenMap.set("xoxb-files-read-token", { login: "U000000001", id: 1, scopes: ["files:read"] });

packages/@emulators/slack/src/routes/files.ts

@@ -75,6 +75,18 @@ export function filesRoutes(ctx: RouteContext): void {
       .channels.all()
       .find((ch) => !ch.is_im && !ch.is_mpim && ch.name === channel);
 
+  type ShareTargetRef = { key: string; channel?: SlackChannel; directUserId?: string };
+
+  const findDirectMessage = (authUserId: string, userId: string) => {
+    const members = [authUserId, userId].sort();
+    return ss()
+      .channels.all()
+      .find(
+        (ch) =>
+          ch.is_im && ch.members.length === members.length && [...ch.members].sort().join(",") === members.join(","),
+      );
+  };
+
   const findOrCreateDirectMessage = (authUser: { login: string }, userId: string) => {
     const targetUser = ss().users.findOneBy("user_id", userId);
     if (!targetUser || targetUser.deleted) return undefined;
@@ -83,12 +95,7 @@ export function filesRoutes(ctx: RouteContext): void {
     if (targetUser.user_id === authUserId) return undefined;
 
     const members = [authUserId, targetUser.user_id].sort();
-    const existing = ss()
-      .channels.all()
-      .find(
-        (ch) =>
-          ch.is_im && ch.members.length === members.length && [...ch.members].sort().join(",") === members.join(","),
-      );
+    const existing = findDirectMessage(authUserId, targetUser.user_id);
     if (existing) return existing;
 
     const team = ss().teams.all()[0];
@@ -113,8 +120,21 @@ export function filesRoutes(ctx: RouteContext): void {
     });
   };
 
-  const findShareTarget = (authUser: { login: string }, channel: string) =>
-    findChannel(channel) ?? (channel.startsWith("U") ? findOrCreateDirectMessage(authUser, channel) : undefined);
+  const resolveShareTarget = (authUser: { login: string }, channel: string): ShareTargetRef | undefined => {
+    const existingChannel = findChannel(channel);
+    if (existingChannel) return { key: existingChannel.channel_id, channel: existingChannel };
+    if (!channel.startsWith("U")) return undefined;
+
+    const targetUser = ss().users.findOneBy("user_id", channel);
+    if (!targetUser || targetUser.deleted) return undefined;
+
+    const authUserId = getAuthUserId(authUser);
+    if (targetUser.user_id === authUserId) return undefined;
+
+    const existingDirectMessage = findDirectMessage(authUserId, targetUser.user_id);
+    if (existingDirectMessage) return { key: existingDirectMessage.channel_id, channel: existingDirectMessage };
+    return { key: `user:${targetUser.user_id}`, directUserId: targetUser.user_id };
+  };
 
   app.post("/api/files.getUploadURLExternal", async (c) => {
     const authUser = c.get("authUser");
@@ -179,16 +199,6 @@ export function filesRoutes(ctx: RouteContext): void {
     }
 
     const authUserId = getAuthUserId(authUser);
-    const rawChannelIds = parseDestinationChannels(body.channel_id, body.channels);
-    const targets: SlackChannel[] = [];
-    for (const channelId of rawChannelIds) {
-      const channel = findShareTarget(authUser, channelId);
-      if (!channel) return slackError(c, "channel_not_found");
-      if (channel.is_archived) return slackError(c, "is_archived");
-      if (!canReadConversation(channel, getAuthSlackUser(authUser), authUserId)) return slackError(c, "not_in_channel");
-      targets.push(channel);
-    }
-
     const initialComment = typeof body.initial_comment === "string" ? body.initial_comment : "";
     const threadTs = typeof body.thread_ts === "string" ? body.thread_ts : undefined;
     const blocks = parseBlocks(body.blocks);
@@ -203,6 +213,29 @@ export function filesRoutes(ctx: RouteContext): void {
       requestedSessions.push(session);
     }
 
+    const rawChannelIds = parseDestinationChannels(body.channel_id, body.channels);
+    const targetRefs: ShareTargetRef[] = [];
+    const targetKeys = new Set<string>();
+    for (const channelId of rawChannelIds) {
+      const target = resolveShareTarget(authUser, channelId);
+      if (!target) return slackError(c, "channel_not_found");
+      if (target.channel?.is_archived) return slackError(c, "is_archived");
+      if (target.channel && !canReadConversation(target.channel, getAuthSlackUser(authUser), authUserId)) {
+        return slackError(c, "not_in_channel");
+      }
+      if (!targetKeys.has(target.key)) {
+        targetKeys.add(target.key);
+        targetRefs.push(target);
+      }
+    }
+
+    const targets: SlackChannel[] = [];
+    for (const target of targetRefs) {
+      const channel = target.channel ?? findOrCreateDirectMessage(authUser, target.directUserId ?? "");
+      if (!channel) return slackError(c, "channel_not_found");
+      targets.push(channel);
+    }
+
     const completedFiles: SlackFile[] = [];
     for (let index = 0; index < requestedFiles.length; index++) {
       const requestedFile = requestedFiles[index];