From 2fb577015b780d817638609c1e91b4555755ce85 Mon Sep 17 00:00:00 2001 From: jlowapik Date: Sun, 6 Sep 2026 16:11:13 +0300 Subject: [PATCH 1/2] fix(slack): paginate the member channel lookup; structure member-rule denials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two further CodeRabbit findings, both verified against the code first. lookupChannelByName read only page one of users.conversations and ignored its cursor. That is not a performance detail: users.conversations is the *only* source that returns im and mpim conversations — the workspace fallback queries public_channel,private_channel and cannot see them at all — so a DM or group DM past the first 200 member conversations was simply unfindable by name. It now follows the cursor under the same page bound as the workspace scan, and reports a bounded member scan rather than falling through and reporting the channel as absent, which would read as "no such channel" when it means "I stopped looking". assertDmMemberAccess still threw a bare UserError, so diagnoseChannelAccess hit its non-denial branch and printed the raw sentence instead of routing through explainDenial — the one tool whose entire purpose is naming the rule and its remedy was the one place these three rules got no remedy. They now throw SlackAccessDenied with dm-org-not-allowed / group-dm-blacklist / group-dm-org and carry the offending team ID, so the explanation names the organisation and points at the right dashboard control. Message text is unchanged, so existing assertions and any caller that only prints them are unaffected. Org-name resolution keys off orgIds being present rather than the single org-not-allowed reason, so the two new org cases get named orgs too. The remaining bare-UserError branch in the tool is now genuinely for non-denial errors only (a missing rules record, say) and says "Could not evaluate" rather than dressing the failure up as a rule verdict. Co-Authored-By: Claude Opus 5 (1M context) --- .../slack-user/listChannelRows.test.ts | 120 +++++++++++++++++- src/slack-user/accessControl.ts | 18 ++- src/slack-user/server.ts | 60 +++++++-- 3 files changed, 178 insertions(+), 20 deletions(-) diff --git a/src/__tests__/slack-user/listChannelRows.test.ts b/src/__tests__/slack-user/listChannelRows.test.ts index 774e7e3..ec648f6 100644 --- a/src/__tests__/slack-user/listChannelRows.test.ts +++ b/src/__tests__/slack-user/listChannelRows.test.ts @@ -195,18 +195,26 @@ describe('renderListSummary', () => { // === diagnoseChannelAccess helpers === import { lookupChannelByName, explainDenial, describeChannel } from '../../slack-user/server.js'; -import { assertAccess, SlackAccessDenied } from '../../slack-user/accessControl.js'; +import { assertAccess, assertDmMemberAccess, SlackAccessDenied } from '../../slack-user/accessControl.js'; import type { ChannelMeta } from '../../slack-user/accessControl.js'; +/** `mine` may be a flat list (one page) or a list of member pages. */ function lookupClient(mine: any[], workspacePages: any[][] = []) { - let page = 0; + const memberPages: any[][] = Array.isArray(mine[0]) ? mine as any[][] : [mine]; + let mp = 0; + let wp = 0; return { - conversationsList: async () => ({ channels: mine }), + conversationsList: async () => { + const channels = memberPages[mp] ?? []; + const more = mp < memberPages.length - 1; + mp++; + return { channels, response_metadata: more ? { next_cursor: `m${mp}` } : {} }; + }, conversationsListAll: async () => { - const channels = workspacePages[page] ?? []; - const more = page < workspacePages.length - 1; - page++; - return { channels, response_metadata: more ? { next_cursor: `c${page}` } : {} }; + const channels = workspacePages[wp] ?? []; + const more = wp < workspacePages.length - 1; + wp++; + return { channels, response_metadata: more ? { next_cursor: `c${wp}` } : {} }; }, } as any; } @@ -396,3 +404,101 @@ describe('renderChannelLines', () => { assert.equal(line, '#bare (C2)\n Type: public'); }); }); + +describe('lookupChannelByName pagination', () => { + it('follows the member-list cursor to find a DM on a later page', async () => { + // users.conversations is the ONLY source that returns im/mpim — the + // workspace fallback cannot see them — so not following its cursor made a + // DM past page one unfindable, not merely slower. + const client = lookupClient([ + [{ id: 'D1', name: 'someone-else', is_im: true }], + [{ id: 'D2', name: 'alice', is_im: true }], + ]); + assert.deepEqual(await lookupChannelByName(client, 'alice'), { kind: 'found', channelId: 'D2' }); + }); + + it('finds a group DM on a later member page', async () => { + const client = lookupClient([ + [{ id: 'G1', name: 'mpdm-x', is_mpim: true }], + [{ id: 'G2', name: 'mpdm-target', is_mpim: true }], + ]); + assert.deepEqual(await lookupChannelByName(client, 'mpdm-target'), { kind: 'found', channelId: 'G2' }); + }); + + it('reports a bounded member scan instead of falling through to a wrong answer', async () => { + const pages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const res = await lookupChannelByName(lookupClient(pages), 'missing'); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).pages, 10); + }); + + it('stops paginating members as soon as it matches', async () => { + let calls = 0; + const client: any = { + conversationsList: async () => { + calls++; + return { channels: [{ id: 'C1', name: 'found-it' }], response_metadata: { next_cursor: 'more' } }; + }, + conversationsListAll: async () => ({ channels: [], response_metadata: {} }), + }; + await lookupChannelByName(client, 'found-it'); + assert.equal(calls, 1); + }); + + it('still falls back to the workspace list for a channel you are not in', async () => { + const client = lookupClient([[]], [[{ id: 'C9', name: 'eng-general' }]]); + assert.deepEqual(await lookupChannelByName(client, 'eng-general'), { kind: 'found', channelId: 'C9' }); + }); +}); + +describe('explainDenial for member rules', () => { + const rulesWithOrg: SlackAccessRules = { + allowedOrgs: ['T_MINE'], blacklistUsers: ['U_BAD'], + whitelistChannels: ['*'], blacklistChannels: [], allowPublicOnly: false, + }; + + async function dmDenial(meta: any, client: any): Promise { + try { await assertDmMemberAccess(client, rulesWithOrg, meta, meta.id ?? 'D1'); } + catch (e) { return e as SlackAccessDenied; } + throw new Error('expected a denial'); + } + + it('names the DM counterpart\'s organisation and the fix', async () => { + const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) }; + const d = await dmDenial({ is_im: true, user: 'U1' }, client); + assert.equal(d.reason, 'dm-org-not-allowed'); + assert.deepEqual(d.orgIds, ['T_OTHER']); + const out = explainDenial(d, rulesWithOrg, '', new Map([['T_OTHER', 'Acme Corp']])).join('\n'); + assert.match(out, /The other participant belongs to an organisation that is not allowed: Acme Corp \(T_OTHER\)/); + assert.match(out, /Access Rules → Organizations/); + }); + + it('names a group DM member\'s organisation', async () => { + const client: any = { + conversationsMembers: async () => ({ members: ['U1'] }), + usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }), + }; + const d = await dmDenial({ is_mpim: true, id: 'G1' }, client); + assert.equal(d.reason, 'group-dm-org'); + const out = explainDenial(d, rulesWithOrg, '').join('\n'); + assert.match(out, /A member of this group DM belongs to an organisation that is not allowed/); + }); + + it('points a blocked group-DM member at the blocked-users list', async () => { + const client: any = { + conversationsMembers: async () => ({ members: ['U_BAD'] }), + usersInfo: async () => ({ user: { id: 'U_BAD', team_id: 'T_MINE' } }), + }; + const d = await dmDenial({ is_mpim: true, id: 'G1' }, client); + assert.equal(d.reason, 'group-dm-blacklist'); + const out = explainDenial(d, rulesWithOrg, '').join('\n'); + assert.match(out, /blocked-users list/); + assert.match(out, /Access Rules → Blocked users/); + }); + + it('degrades to the raw ID when the org cannot be named', async () => { + const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) }; + const d = await dmDenial({ is_im: true, user: 'U1' }, client); + assert.match(explainDenial(d, rulesWithOrg, '').join('\n'), /not allowed: T_OTHER/); + }); +}); diff --git a/src/slack-user/accessControl.ts b/src/slack-user/accessControl.ts index 551fb7f..0a93f92 100644 --- a/src/slack-user/accessControl.ts +++ b/src/slack-user/accessControl.ts @@ -22,6 +22,9 @@ import type { SlackAccessRules } from '../mcpConnectionStore.js'; export type SlackDenialReason = | 'org-not-allowed' | 'org-unverified' + | 'dm-org-not-allowed' + | 'group-dm-blacklist' + | 'group-dm-org' | 'whitelist-empty' | 'whitelist-miss' | 'blacklist-channel' @@ -242,7 +245,10 @@ export async function assertDmMemberAccess( if (meta.is_im && meta.user && rules.allowedOrgs.length > 0) { const { user } = await client.usersInfo(meta.user); if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) { - throw new UserError('Access denied: this user belongs to an organisation not in your allowed list.'); + throw new SlackAccessDenied( + 'Access denied: this user belongs to an organisation not in your allowed list.', + { reason: 'dm-org-not-allowed', orgIds: [user.team_id] }, + ); } } @@ -250,13 +256,19 @@ export async function assertDmMemberAccess( if (meta.is_mpim && (rules.blacklistUsers.length > 0 || rules.allowedOrgs.length > 0)) { const { members } = await client.conversationsMembers(channelId); if (rules.blacklistUsers.length > 0 && members.some(uid => rules.blacklistUsers.includes(uid))) { - throw new UserError('Access denied: this group DM contains a blacklisted user.'); + throw new SlackAccessDenied( + 'Access denied: this group DM contains a blacklisted user.', + { reason: 'group-dm-blacklist' }, + ); } if (rules.allowedOrgs.length > 0) { for (const uid of members) { const { user } = await client.usersInfo(uid); if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) { - throw new UserError('Access denied: this group DM contains a user from a non-allowed organisation.'); + throw new SlackAccessDenied( + 'Access denied: this group DM contains a user from a non-allowed organisation.', + { reason: 'group-dm-org', orgIds: [user.team_id] }, + ); } } } diff --git a/src/slack-user/server.ts b/src/slack-user/server.ts index 0ad95a7..9b608af 100644 --- a/src/slack-user/server.ts +++ b/src/slack-user/server.ts @@ -188,6 +188,9 @@ const DENIAL_LABELS: Record = { 'blacklist-channel': 'blacklist', 'blacklist-user': 'blocked user', 'dm-rules': 'DM/group-DM rules (blocked user or organisation)', + 'dm-org-not-allowed': 'organisation of the other participant', + 'group-dm-blacklist': 'blocked user in the group DM', + 'group-dm-org': 'organisation of a group DM member', 'public-only': 'private (allowPublicOnly)', 'org-not-allowed': 'organisation', 'org-unverified': 'organisation unverified', @@ -972,23 +975,38 @@ export async function lookupChannelByName( const matches = (channels: ListedChannel[]) => channels.filter(ch => ch.name?.toLowerCase() === wanted); - const mine = await client.conversationsList(undefined, 'public_channel,private_channel,mpim,im'); - let hits = matches(mine.channels as ListedChannel[]); - + // Pass 1: users.conversations, paginated. It is member-scoped, so it is both + // cheap and the *only* source that returns DMs and group DMs — the workspace + // fallback below cannot see them at all. Following its cursor therefore is + // not an optimisation: without it, a DM on page two is simply unfindable. + // No initialiser: the do-while below always assigns before anything reads it. + let hits: ListedChannel[]; let cursor: string | undefined; - let pages = 0; - while (hits.length === 0 && pages < DIAGNOSE_MAX_PAGES) { + let memberPages = 0; + do { + const page = await client.conversationsList(cursor, 'public_channel,private_channel,mpim,im'); + hits = matches(page.channels as ListedChannel[]); + cursor = page.response_metadata?.next_cursor || undefined; + memberPages++; + } while (hits.length === 0 && cursor && memberPages < DIAGNOSE_MAX_PAGES); + + if (hits.length === 0 && cursor) return { kind: 'scanBounded', pages: memberPages }; + + // Pass 2: the workspace-wide list, for a channel the user is not in. + let workspacePages = 0; + cursor = undefined; + while (hits.length === 0 && workspacePages < DIAGNOSE_MAX_PAGES) { const page = await client.conversationsListAll(cursor, 'public_channel,private_channel'); hits = matches(page.channels as ListedChannel[]); cursor = page.response_metadata?.next_cursor || undefined; - pages++; + workspacePages++; if (!cursor) break; } if (hits.length === 1) return { kind: 'found', channelId: hits[0].id }; if (hits.length > 1) return { kind: 'ambiguous', ids: hits.map(h => h.id) }; // A bounded scan is a floor, not a verdict. - return cursor ? { kind: 'scanBounded', pages } : { kind: 'none' }; + return cursor ? { kind: 'scanBounded', pages: workspacePages } : { kind: 'none' }; } /** @@ -1015,6 +1033,25 @@ export function explainDenial( ); break; } + case 'dm-org-not-allowed': + case 'group-dm-org': { + const labels = (denial.orgIds ?? []).map(id => formatTeamLabel(id, orgNames)).join(', '); + const who = denial.reason === 'dm-org-not-allowed' + ? 'The other participant belongs to' + : 'A member of this group DM belongs to'; + out.push( + `${who} an organisation that is not allowed: ${labels || '(unresolved)'}`, + `Your allowed organisations: ${rules.allowedOrgs.length ? rules.allowedOrgs.join(', ') : '(none)'}`, + 'Fix: tick the organisation under Access Rules → Organizations in the dashboard, or remove the person from the conversation.', + ); + break; + } + case 'group-dm-blacklist': + out.push( + 'One of this group DM\'s members is on your blocked-users list.', + 'Fix: remove them under Access Rules → Blocked users, or leave the conversation.', + ); + break; case 'org-unverified': out.push( 'Slack returned no organisation for this shared channel, so it cannot be checked against your allowlist.', @@ -1111,7 +1148,10 @@ slackUserServer.addTool({ await assertDmMemberAccess(client, rules, meta, channelId!); } catch (err) { if (err instanceof SlackAccessDenied) denial = err; - else if (err instanceof UserError) return [header, '', `Denied by: ${err.message}`].join('\n'); + // Every access rule now throws SlackAccessDenied, so this branch is only + // for a UserError that is not a denial at all — a missing rules record, + // say. Report it rather than dressing it up as a rule verdict. + else if (err instanceof UserError) return [header, '', `Could not evaluate: ${err.message}`].join('\n'); else throw err; } @@ -1119,8 +1159,8 @@ slackUserServer.addTool({ return [header, '', 'Readable: yes. Every access rule passes for this channel.'].join('\n'); } - const orgNames = denial.reason === 'org-not-allowed' - ? (await resolveTeamNames(client, denial.orgIds ?? [], { tokenKey })).names + const orgNames = (denial.orgIds?.length ?? 0) > 0 + ? (await resolveTeamNames(client, denial.orgIds!, { tokenKey })).names : undefined; return [header, '', ...explainDenial(denial, rules, meta.name, orgNames)].join('\n'); }, From 753cccb1978e3c7b751b00de7f6814d003ff4f29 Mon Sep 17 00:00:00 2001 From: jlowapik Date: Sun, 6 Sep 2026 16:29:45 +0300 Subject: [PATCH 2/2] fix(slack): keep the workspace fallback after a bounded member scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression introduced by the pagination fix in this PR. Returning early when the member scan hit its page bound skipped conversationsListAll entirely, so an account with more than DIAGNOSE_MAX_PAGES of member conversations could no longer discover a public or private channel it is not a member of — the exact case the workspace fallback exists for. The bound is now remembered rather than returned: the workspace scan always runs, and only if neither scan finds the channel does the uncertainty surface. ChannelLookup's scanBounded variant carries a `source`, because the caller was reporting a member-scan limit as "the first N pages of the workspace channel list" — naming the wrong list, which sends the user looking in the wrong place. Co-Authored-By: Claude Opus 5 (1M context) --- .../slack-user/listChannelRows.test.ts | 32 +++++++++++++++++++ src/slack-user/server.ts | 31 ++++++++++++++---- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/src/__tests__/slack-user/listChannelRows.test.ts b/src/__tests__/slack-user/listChannelRows.test.ts index ec648f6..c0931f8 100644 --- a/src/__tests__/slack-user/listChannelRows.test.ts +++ b/src/__tests__/slack-user/listChannelRows.test.ts @@ -247,6 +247,7 @@ describe('lookupChannelByName', () => { const res = await lookupChannelByName(client, 'missing'); assert.equal(res.kind, 'scanBounded'); assert.equal((res as any).pages, 10); + assert.equal((res as any).source, 'workspace'); }); it('reports a genuine absence when the scan reached the end', async () => { @@ -430,6 +431,7 @@ describe('lookupChannelByName pagination', () => { const res = await lookupChannelByName(lookupClient(pages), 'missing'); assert.equal(res.kind, 'scanBounded'); assert.equal((res as any).pages, 10); + assert.equal((res as any).source, 'member'); }); it('stops paginating members as soon as it matches', async () => { @@ -502,3 +504,33 @@ describe('explainDenial for member rules', () => { assert.match(explainDenial(d, rulesWithOrg, '').join('\n'), /not allowed: T_OTHER/); }); }); + +describe('lookupChannelByName workspace fallback after a bounded member scan', () => { + it('still finds a workspace channel when the member scan ran out of pages', async () => { + // The regression: returning early on the member bound meant an account with + // enough member conversations could never reach the workspace list, so a + // public channel it is not a member of became undiscoverable. + const memberPages = Array.from({ length: 11 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const client = lookupClient(memberPages, [[{ id: 'C_TARGET', name: 'eng-general' }]]); + assert.deepEqual(await lookupChannelByName(client, 'eng-general'), { + kind: 'found', channelId: 'C_TARGET', + }); + }); + + it('reports both limits when neither scan finished', async () => { + const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const workspacePages = Array.from({ length: 12 }, (_, i) => [{ id: `C${i}`, name: `ch-${i}` }]); + const res = await lookupChannelByName(lookupClient(memberPages, workspacePages), 'missing'); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).source, 'both'); + }); + + it('reports only the member limit when the workspace list was exhausted', async () => { + const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const res = await lookupChannelByName( + lookupClient(memberPages, [[{ id: 'C1', name: 'something-else' }]]), 'missing', + ); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).source, 'member'); + }); +}); diff --git a/src/slack-user/server.ts b/src/slack-user/server.ts index 9b608af..ef57a16 100644 --- a/src/slack-user/server.ts +++ b/src/slack-user/server.ts @@ -954,7 +954,8 @@ const DIAGNOSE_MAX_PAGES = 10; export type ChannelLookup = | { kind: 'found'; channelId: string } | { kind: 'none' } - | { kind: 'scanBounded'; pages: number } + /** Which scan ran out of pages — the caller has to name the right one. */ + | { kind: 'scanBounded'; pages: number; source: 'member' | 'workspace' | 'both' } | { kind: 'ambiguous'; ids: string[] }; /** @@ -990,7 +991,11 @@ export async function lookupChannelByName( memberPages++; } while (hits.length === 0 && cursor && memberPages < DIAGNOSE_MAX_PAGES); - if (hits.length === 0 && cursor) return { kind: 'scanBounded', pages: memberPages }; + // Hitting the member bound must NOT skip the workspace scan: a public channel + // the user is not in is only ever found there, and an account with enough + // member conversations would otherwise never reach it. Remember the + // uncertainty and carry on. + const memberScanBounded = hits.length === 0 && !!cursor; // Pass 2: the workspace-wide list, for a channel the user is not in. let workspacePages = 0; @@ -1005,8 +1010,16 @@ export async function lookupChannelByName( if (hits.length === 1) return { kind: 'found', channelId: hits[0].id }; if (hits.length > 1) return { kind: 'ambiguous', ids: hits.map(h => h.id) }; - // A bounded scan is a floor, not a verdict. - return cursor ? { kind: 'scanBounded', pages: workspacePages } : { kind: 'none' }; + + // A bounded scan is a floor, not a verdict — and the caller has to be told + // which list ran out, since the two mean different things to the user. + const workspaceScanBounded = !!cursor; + if (memberScanBounded && workspaceScanBounded) { + return { kind: 'scanBounded', pages: Math.max(memberPages, workspacePages), source: 'both' }; + } + if (workspaceScanBounded) return { kind: 'scanBounded', pages: workspacePages, source: 'workspace' }; + if (memberScanBounded) return { kind: 'scanBounded', pages: memberPages, source: 'member' }; + return { kind: 'none' }; } /** @@ -1129,11 +1142,17 @@ slackUserServer.addTool({ case 'ambiguous': return [`${found.ids.length} channels are named "${args.name}". Re-run with one of these IDs:`, ...found.ids.map(id => ` ${id}`)].join('\n'); - case 'scanBounded': + case 'scanBounded': { + const which = { + member: 'your own conversations', + workspace: 'the workspace channel list', + both: 'both your own conversations and the workspace channel list', + }[found.source]; return [ - `No channel named "${args.name}" found in the first ${found.pages} page(s) of the workspace channel list.`, + `No channel named "${args.name}" found in the first ${found.pages} page(s) of ${which}.`, 'The scan stopped at its page limit, so the channel may still exist further in. Pass channelId to check it directly.', ].join('\n'); + } default: return `No channel named "${args.name}" is visible to your Slack account. Slack itself does not return it, so this is not an access-rules problem — you are most likely not a member of it.`; }