Skip to content

Commit 33b389e

Browse files
committed
fix(slack): grant im:read/mpim:read for DM discovery, guard listDMChannels
conversations.list with types=im,mpim (used to discover DM/MPIM conversation ids) requires im:read/mpim:read, not im:history/mpim:history — those only grant reading message content within an already-known conversation. The "Sync direct messages" optional scope group never requested im:read/mpim:read, so listDMChannels got missing_scope on every call and never actually discovered incoming DM conversations. Worse, listDMChannels didn't guard on the DM scope before calling the API (unlike its siblings syncCustomEmoji/registerDMWebhook), so a user who declined DM sync would still hit missing_scope, which is in the permanent auth-error set — force-flagging the whole connection as needing reconnection even though nothing was broken. Add im:read/mpim:read to the dms scope group, and add the same token.scopes-based guard used by syncCustomEmoji/registerDMWebhook so a declined scope short-circuits before any API call.
1 parent 2127cee commit 33b389e

2 files changed

Lines changed: 84 additions & 7 deletions

File tree

‎connectors/slack/src/slack.test.ts‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,10 @@ describe("listDMChannels", () => {
421421

422422
it("caches discovered im/mpim channel ids and gates re-runs to once per 24h", async () => {
423423
const store = makeStore({});
424-
const integrationsGet = vi.fn().mockResolvedValue({ token: "xoxp-test" });
424+
const integrationsGet = vi.fn().mockResolvedValue({
425+
token: "xoxp-test",
426+
scopes: ["im:read", "mpim:read"],
427+
});
425428
const slack = makeDMListSlack({ store, integrationsGet });
426429

427430
const fetchMock = vi.fn(async () =>
@@ -452,6 +455,45 @@ describe("listDMChannels", () => {
452455
vi.unstubAllGlobals();
453456
}
454457
});
458+
459+
it("no-ops when im:read is not granted, without calling the API or flagging re-auth", async () => {
460+
// Guards against the false-positive-reconnect bug class this branch
461+
// otherwise fixes: without this guard, a token missing im:read hits
462+
// conversations.list, gets `missing_scope` back (a SlackPermanentError
463+
// in SLACK_AUTH_ERRORS), and markNeedsReauth force-flags the whole
464+
// connection as needing reconnection even though nothing is broken.
465+
const store = makeStore({});
466+
const integrationsGet = vi.fn().mockResolvedValue({
467+
token: "xoxp-test",
468+
scopes: ["channels:history"], // no im:read
469+
});
470+
const markNeedsReauth = vi.fn().mockResolvedValue(undefined);
471+
const runTask = vi.fn(async () => "task-token");
472+
const create = vi.fn(async () => ({ token: "cb" }) as never);
473+
const tools = {
474+
store,
475+
integrations: { get: integrationsGet, markNeedsReauth },
476+
network: { createWebhook: vi.fn() },
477+
files: {},
478+
callbacks: { create },
479+
tasks: { runTask, scheduleRecurring: vi.fn(async () => {}) },
480+
};
481+
const slack = new Slack("twist-instance-1" as never, { getTools: () => tools } as never);
482+
483+
const fetchMock = vi.fn();
484+
vi.stubGlobal("fetch", fetchMock);
485+
try {
486+
await (slack as unknown as {
487+
listDMChannels: (c: string) => Promise<void>;
488+
}).listDMChannels("C1");
489+
} finally {
490+
vi.unstubAllGlobals();
491+
}
492+
493+
expect(fetchMock).not.toHaveBeenCalled();
494+
expect(markNeedsReauth).not.toHaveBeenCalled();
495+
expect(store.map.has("dm_channels")).toBe(false);
496+
});
455497
});
456498

457499
describe("extractSlackMessageReactions (custom emoji)", () => {
@@ -717,6 +759,17 @@ describe("Slack.SCOPES — DM sync is optional and opt-in", () => {
717759
);
718760
expect(dmsGroup?.default).toBe(true);
719761
});
762+
763+
it("declares im:read and mpim:read in the dms group, required to enumerate/list DM conversations", () => {
764+
// conversations.list with types=im,mpim (used by listDMChannels to
765+
// discover DM/MPIM conversation ids) requires im:read/mpim:read per
766+
// Slack's API docs — distinct from im:history/mpim:history, which only
767+
// grant reading message content within an already-known conversation.
768+
// Without these, listDMChannels gets `missing_scope` on every call.
769+
const dmsGroup = Slack.SCOPES.optional?.find((g) => g.id === "dms");
770+
expect(dmsGroup).toBeDefined();
771+
expect(dmsGroup?.scopes).toEqual(expect.arrayContaining(["im:read", "mpim:read"]));
772+
});
720773
});
721774

722775
describe("onChannelEnabled — workspace daily task dedup", () => {

‎connectors/slack/src/slack.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,12 @@ import { slackFacets } from "./slack-facets";
8282
* - `emoji:read` — render the workspace's custom emoji in reactions
8383
* - `im:history`, `im:write`, `mpim:history`, `mpim:write` — read and compose
8484
* direct messages and group DMs
85+
* - `im:read`, `mpim:read` — enumerate/list the user's DM and group-DM
86+
* conversations (`conversations.list` with `types=im,mpim`), which is how
87+
* {@link listDMChannels} discovers DM/MPIM conversation ids. Distinct from
88+
* `im:history`/`mpim:history`, which only grant reading message content
89+
* within a conversation whose id is already known — they do not grant
90+
* enumeration, so `listDMChannels` needs both.
8591
*/
8692

8793
/**
@@ -147,7 +153,17 @@ export class Slack extends Connector<Slack> {
147153
label: "Sync direct messages",
148154
description:
149155
"Bring your Slack DMs and group DMs into Plot, and let you send new ones from Plot.",
150-
scopes: ["im:history", "im:write", "mpim:history", "mpim:write"],
156+
scopes: [
157+
"im:history",
158+
"im:write",
159+
"mpim:history",
160+
"mpim:write",
161+
// Enumeration scopes: conversations.list with types=im,mpim (used by
162+
// listDMChannels to discover conversation ids) requires these, not
163+
// the *:history scopes above — see the class doc comment.
164+
"im:read",
165+
"mpim:read",
166+
],
151167
default: true,
152168
},
153169
],
@@ -1665,20 +1681,28 @@ export class Slack extends Connector<Slack> {
16651681
* mirroring {@link syncMembers} / {@link syncCustomEmoji}. A brand-new
16661682
* incoming DM conversation is picked up on the next daily run, or
16671683
* immediately if proactively registered (see `createDirectMessage`).
1684+
*
1685+
* Requires the `im:read` optional scope (part of the `dms` group) to have
1686+
* been granted — no-ops otherwise (the user declined DM sync at connect
1687+
* time). Without this guard, a token missing `im:read` would hit
1688+
* `conversations.list`, get back `missing_scope` (a permanent error in
1689+
* `SLACK_AUTH_ERRORS`), and force-flag the entire connection as needing
1690+
* reconnection even though nothing is actually broken.
16681691
*/
16691692
async listDMChannels(channelId: string): Promise<void> {
16701693
const now = Date.now();
16711694
const ONE_DAY_MS = 24 * 60 * 60 * 1000;
16721695
const last = await this.get<number>("dmChannelsSyncedAt");
16731696
if (last && now - last < ONE_DAY_MS) return;
16741697

1675-
let api: SlackApi;
1676-
try {
1677-
api = await this.getApi(channelId);
1678-
} catch (error) {
1679-
console.warn("listDMChannels: Slack token unavailable", error);
1698+
const token = await this.tools.integrations.get(channelId);
1699+
if (!token) {
1700+
console.warn("listDMChannels: Slack token unavailable");
16801701
return;
16811702
}
1703+
if (!token.scopes?.includes("im:read")) return; // optional scope declined
1704+
1705+
const api = new SlackApi(token.token);
16821706

16831707
const ids: string[] = [];
16841708
let cursor: string | undefined;

0 commit comments

Comments
 (0)