From 6b7b5a4945c2e43438fbe011e70d62969e41d2ea Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:01:30 -0800 Subject: [PATCH 1/3] fix(discord): the install link returns to the callback, and Connectors offers Discord The bot-add link had no redirect_uri and no response_type=code, so Discord added the bot and never came back: no callback, no install proof, no binding. The link is now a code grant whose redirect comes from the same helper the callback's token exchange uses, and it names only this instance's client id. v2 Connectors lists Discord when the instance holds its keys, even though no builtin Installable row exists for it, and offers it beside Telegram and Slack in Connect a channel. Its Connect goes to the install link rather than the Installable lifecycle. A service test walks install-link, callback, channels on the proof, POST /api/integrations and GET /binding against a real mongod. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DT6tbhKm6UhsmFhLk9 --- .../service/discordInstallBind.test.js | 204 ++++++++++++++++++ backend/routes/discord.ts | 31 ++- frontend/src/i18n/locales/en.json | 2 + frontend/src/i18n/locales/zh-CN.json | 2 + .../ConnectorsPage.channels.test.tsx | 116 ++++++++-- frontend/src/v2/connectors/ConnectorsPage.tsx | 4 +- frontend/src/v2/connectors/channelModel.ts | 16 ++ .../src/v2/connectors/useChannelConnectors.ts | 32 ++- 8 files changed, 381 insertions(+), 26 deletions(-) create mode 100644 backend/__tests__/service/discordInstallBind.test.js diff --git a/backend/__tests__/service/discordInstallBind.test.js b/backend/__tests__/service/discordInstallBind.test.js new file mode 100644 index 0000000000..dd1b4a3a29 --- /dev/null +++ b/backend/__tests__/service/discordInstallBind.test.js @@ -0,0 +1,204 @@ +// Discord install and bind: a Discord bot add started from Commonly's install +// link must come back through `/api/discord/callback`, and the success page's +// two requests must then leave a bound pod. On prod 11-04 the link carried no +// `redirect_uri` and no `response_type=code`, so Discord added the bot and never +// redirected: no callback, no install proof, no Integration, and `GET /binding` +// stayed 404. This walks the whole chain against a real mongod, with Discord's +// HTTP API stubbed at axios: +// +// install-link -> Discord authorize (asserted as a URL) -> callback(code) +// -> /discord/success?install= -> channels(proof) -> POST integrations +// -> GET binding +// +// The redirect the link names must be the one the callback exchanges the code +// with: Discord refuses a token exchange whose redirect_uri differs from the +// authorize request's. +/* eslint-disable global-require */ +const express = require('express'); +const request = require('supertest'); +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); + +process.env.PG_HOST = ''; + +const OWNER = '6a8f6de2a1dccf2e02f31401'; +const MEMBER = '6a8f6de2a1dccf2e02f31402'; +const POD_ID = '6a8f6de2a1dccf2e02f31499'; +const GUILD = '123456789012345678'; +const CHANNEL = '234567890123456789'; +const WEBHOOK = { id: '345678901234567890', token: 'webhook-token' }; + +jest.mock('../../middleware/auth', () => (req, _res, next) => { + const token = String(req.header('Authorization') || '').replace('Bearer ', ''); + req.user = { id: token === 'member-token' ? '6a8f6de2a1dccf2e02f31402' : '6a8f6de2a1dccf2e02f31401' }; + req.userId = req.user.id; + next(); +}); +jest.mock('../../middleware/adminAuth', () => (_req, _res, next) => next()); +jest.mock('../../middleware/integrationRateLimit', () => ({ + writeIntegrationsRateLimit: (_req, _res, next) => next(), + listIntegrationsRateLimit: (_req, _res, next) => next(), + ingestIngressRateLimit: (_req, _res, next) => next(), + ingestIntegrationsRateLimit: (_req, _res, next) => next(), +})); +jest.mock('axios', () => ({ get: jest.fn(), post: jest.fn() })); +jest.mock('../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../models/Pod', () => ({ findById: jest.fn(), find: jest.fn() })); +jest.mock('../../services/discordService', () => jest.fn().mockImplementation(() => ({ + initialize: jest.fn().mockResolvedValue(true), + connect: jest.fn().mockResolvedValue(true), +}))); +jest.mock('../../services/connectorSecrets', () => ({ + put: jest.fn().mockResolvedValue('secret-ref-1'), + get: jest.fn(), + remove: jest.fn(), +})); + +let mongod; +let app; +let axios; +let Integration; +let DiscordIntegration; +let User; +let Pod; + +const POD = { + _id: POD_ID, name: 'Discord walk', type: 'team', createdBy: OWNER, members: [OWNER, MEMBER], +}; + +beforeAll(async () => { + process.env.JWT_SECRET = 'test-secret'; + process.env.DISCORD_CLIENT_ID = '1111111111'; + process.env.DISCORD_CLIENT_SECRET = 'client-secret'; + process.env.DISCORD_BOT_TOKEN = 'bot-token'; + process.env.BACKEND_URL = 'https://api.example.test'; + process.env.FRONTEND_URL = 'https://app.example.test'; + mongod = await MongoMemoryServer.create(); + await mongoose.connect(mongod.getUri()); + axios = require('axios'); + Integration = require('../../models/Integration'); + DiscordIntegration = require('../../models/DiscordIntegration'); + User = require('../../models/User'); + Pod = require('../../models/Pod'); + await Integration.syncIndexes(); + app = express(); + app.use(express.json()); + app.use('/api/discord', require('../../routes/discord')); + app.use('/api/integrations', require('../../routes/integrations')); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongod.stop(); +}); + +beforeEach(async () => { + jest.clearAllMocks(); + await Integration.deleteMany({}); + await DiscordIntegration.deleteMany({}); + User.findById.mockImplementation(async (id) => ({ _id: String(id), role: 'user' })); + Pod.findById.mockImplementation(async (id) => (String(id) === POD_ID ? { ...POD } : null)); + Pod.find.mockImplementation((query) => { + const userId = String(query?.$or?.[0]?.members || ''); + const rows = POD.members.includes(userId) || POD.createdBy === userId ? [{ _id: POD_ID }] : []; + return { select: () => ({ lean: async () => rows }) }; + }); + axios.post.mockImplementation(async (url) => { + if (url === 'https://discord.com/api/oauth2/token') { + return { data: { access_token: 'user-access', guild: { id: GUILD, name: 'Commonly Dev' } } }; + } + if (url === `https://discord.com/api/channels/${CHANNEL}/webhooks`) return { data: WEBHOOK }; + throw new Error(`unexpected POST ${url}`); + }); + axios.get.mockImplementation(async (url) => { + if (url === `https://discord.com/api/guilds/${GUILD}`) return { data: { id: GUILD, name: 'Commonly Dev' } }; + if (url === `https://discord.com/api/guilds/${GUILD}/channels`) { + return { data: [{ id: CHANNEL, name: 'general', type: 0 }, { id: '999999999999999999', name: 'Voice', type: 2 }] }; + } + if (url === `https://discord.com/api/channels/${CHANNEL}`) return { data: { id: CHANNEL, guild_id: GUILD, type: 0 } }; + throw new Error(`unexpected GET ${url}`); + }); +}); + +const CALLBACK = 'https://api.example.test/api/discord/callback'; + +test('the install link sends Discord back to the callback with a code', async () => { + const res = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer owner-token'); + expect(res.status).toBe(200); + const url = new URL(res.body.installUrl); + expect(url.origin + url.pathname).toBe('https://discord.com/api/oauth2/authorize'); + expect(url.searchParams.get('client_id')).toBe('1111111111'); + expect(url.searchParams.get('response_type')).toBe('code'); + expect(url.searchParams.get('redirect_uri')).toBe(CALLBACK); + expect(url.searchParams.get('state')).toBe(`pod_${POD_ID}`); + expect(url.searchParams.get('scope')).toBe('bot applications.commands'); +}); + +test('the link ignores a caller-supplied client id', async () => { + const res = await request(app) + .get(`/api/discord/install-link/${POD_ID}?clientId=2222222222`) + .set('Authorization', 'Bearer owner-token'); + expect(res.status).toBe(200); + expect(new URL(res.body.installUrl).searchParams.get('client_id')).toBe('1111111111'); +}); + +test('a bot add from the link ends in a bound pod', async () => { + const link = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer owner-token'); + const authorize = new URL(link.body.installUrl); + expect((await request(app).get(`/api/discord/binding/${POD_ID}`).set('Authorization', 'Bearer owner-token')).status).toBe(404); + + // Discord redirects the browser to the link's redirect_uri with a code, the + // state it was given and the guild the bot joined. + const back = new URL(authorize.searchParams.get('redirect_uri')); + const cb = await request(app) + .get(`${back.pathname}?code=the-code&state=${authorize.searchParams.get('state')}&guild_id=${GUILD}`); + expect(cb.status).toBe(302); + const success = new URL(cb.headers.location); + expect(success.origin + success.pathname).toBe('https://app.example.test/discord/success'); + expect(success.searchParams.get('pod_id')).toBe(POD_ID); + expect(success.searchParams.get('guild_id')).toBe(GUILD); + const proof = success.searchParams.get('install'); + expect(proof).toBeTruthy(); + // The code exchange names the same redirect the authorize request did. + const exchange = axios.post.mock.calls.find(([url]) => url === 'https://discord.com/api/oauth2/token'); + expect(exchange[1].redirect_uri).toBe(CALLBACK); + + // The success page lists the guild's channels on the proof... + const channels = await request(app) + .get(`/api/discord/channels/${GUILD}?install=${encodeURIComponent(proof)}`) + .set('Authorization', 'Bearer owner-token'); + expect(channels.status).toBe(200); + expect(channels.body.map((c) => c.id)).toContain(CHANNEL); + + // ...and binds the one the person picks. + const bind = await request(app) + .post('/api/integrations') + .set('Authorization', 'Bearer owner-token') + .send({ + podId: POD_ID, + type: 'discord', + config: { + serverId: GUILD, serverName: 'Commonly Dev', channelId: CHANNEL, channelName: 'general', webhookUrl: '', botToken: '', + }, + install: proof, + }); + expect(bind.status).toBe(201); + + const row = await Integration.findOne({ podId: POD_ID, type: 'discord' }).lean(); + expect(row).toBeTruthy(); + expect(String(row.createdBy)).toBe(OWNER); + expect(row.config.serverId).toBe(GUILD); + expect(row.config.channelId).toBe(CHANNEL); + expect(row.config.webhookUrlRef).toBe('secret-ref-1'); + const platform = await DiscordIntegration.findOne({ integrationId: row._id }).lean(); + expect(platform).toMatchObject({ serverId: GUILD, channelId: CHANNEL, webhookId: WEBHOOK.id }); + + const binding = await request(app).get(`/api/discord/binding/${POD_ID}`).set('Authorization', 'Bearer owner-token'); + expect(binding.status).toBe(200); + expect(binding.body.discordIntegration.channelId).toBe(CHANNEL); +}); + +test('a pod member who did not create the pod gets no install link', async () => { + const res = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer member-token'); + expect(res.status).toBe(403); +}); diff --git a/backend/routes/discord.ts b/backend/routes/discord.ts index cd041d3ed6..a2498e6f34 100644 --- a/backend/routes/discord.ts +++ b/backend/routes/discord.ts @@ -63,6 +63,16 @@ function verifySignature(req: { headers: Record; bod } } +// The bot permissions the install asks for (the value the legacy room link +// uses): view channels, send messages, manage webhooks. +const DISCORD_BOT_PERMISSIONS = '536873984'; + +// One redirect for the authorize request and the code exchange: Discord refuses +// an exchange whose redirect_uri differs from the one the user authorized. +const discordRedirectUri = (): string => ( + `${String(process.env.BACKEND_URL || 'http://localhost:5000').replace(/\/+$/, '')}/api/discord/callback` +); + async function canManagePod(podId: string, userId: string): Promise { const [user, pod] = await Promise.all([User.findById(userId), Pod.findById(podId)]) as [{ role?: string } | null, { createdBy?: { toString: () => string } } | null]; if (!user || !pod) return false; @@ -209,13 +219,24 @@ router.post('/interactions', async (req: AuthReq & { body: Buffer }, res: Res) = router.get('/install-link/:podId', auth, async (req: AuthReq, res: Res) => { try { const { podId } = req.params || {}; - const clientId = req.query?.clientId || process.env.DISCORD_CLIENT_ID; + // The instance's own application only. A caller-chosen client id would send + // the bot add to an app whose callback is not ours. + const clientId = process.env.DISCORD_CLIENT_ID; const canManage = await canManagePod(podId || '', req.user?.id || ''); if (!canManage) return res.status(403).json({ error: 'Access denied' }); if (!clientId) return res.status(400).json({ error: 'Discord client ID is required' }); - const baseUrl = 'https://discord.com/api/oauth2/authorize'; - const installUrl = `${baseUrl}?client_id=${clientId}&scope=bot%20applications.commands&permissions=536873984&state=pod_${podId}`; - res.json({ installUrl, podId, status: 'ready' }); + // A code grant with our callback as the redirect. Without `response_type` + // and `redirect_uri`, Discord adds the bot and stops on its own page, so the + // callback never runs: no install proof, no channel picker, no binding (walked + // on prod 2026-11-04). + const installUrl = new URL('https://discord.com/api/oauth2/authorize'); + installUrl.searchParams.set('client_id', String(clientId)); + installUrl.searchParams.set('scope', 'bot applications.commands'); + installUrl.searchParams.set('permissions', DISCORD_BOT_PERMISSIONS); + installUrl.searchParams.set('response_type', 'code'); + installUrl.searchParams.set('redirect_uri', discordRedirectUri()); + installUrl.searchParams.set('state', `pod_${podId}`); + res.json({ installUrl: installUrl.toString(), podId, status: 'ready' }); } catch { console.error('Error generating installation link'); res.status(500).json({ error: 'Failed to generate installation link' }); @@ -336,7 +357,7 @@ router.get('/callback', discordCallbackLimit, async (req: AuthReq, res: Res) => if (isSupplied(guildId) && !isDiscordSnowflake(guildId)) { return res.redirect(`${frontend}/discord/error?error=Invalid guild id`); } - const tokenResponse = await axios.post('https://discord.com/api/oauth2/token', { client_id: process.env.DISCORD_CLIENT_ID, client_secret: process.env.DISCORD_CLIENT_SECRET, grant_type: 'authorization_code', code, redirect_uri: `${process.env.BACKEND_URL || 'http://localhost:5000'}/api/discord/callback` }, { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); + const tokenResponse = await axios.post('https://discord.com/api/oauth2/token', { client_id: process.env.DISCORD_CLIENT_ID, client_secret: process.env.DISCORD_CLIENT_SECRET, grant_type: 'authorization_code', code, redirect_uri: discordRedirectUri() }, { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); // The token response names the guild the bot was added to. `guild_id` in the // query is only Discord's hint and anyone can edit it, so a disagreement is a // refusal, and only the token response's guild earns an install proof (F89). diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index dce54835b6..40efda307d 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2520,6 +2520,8 @@ "availableLine": "Connect {{label}} to a pod.", "availableSlack": "one click in your workspace", "availableTelegram": "one message", + "availableDiscord": "add the bot to your server", + "linkInstallCreatorOnly": "Only the person who created this pod can add the {{connector}} bot to it.", "cancel": "Cancel", "channelDetails": "Channel details", "closeDetails": "Close", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index e1540e3afe..5db387a8c0 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -2508,6 +2508,8 @@ "availableLine": "将 {{label}} 连接到 Pod。", "availableSlack": "在你的工作空间中点一下", "availableTelegram": "发一条消息", + "availableDiscord": "把机器人加进你的服务器", + "linkInstallCreatorOnly": "只有这个 Pod 的创建者才能把 {{connector}} 机器人加进来。", "cancel": "取消", "channelDetails": "频道详情", "closeDetails": "关闭", diff --git a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx index 59e245c501..94ae37365f 100644 --- a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx +++ b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx @@ -1190,18 +1190,18 @@ describe('Connectors page: channels', () => { }); it('offers a rostered provider with Add, and an unmapped one inherits no onboarding sentence', async () => { - // Slack's line is in the map; Discord's is not. The point of the map is that - // Discord renders NO sentence rather than Slack's. + // Slack's line is in the map; GroupMe's is not. The point of the map is that + // GroupMe renders NO sentence rather than Slack's. mockCatalog([ - entry({ installableId: 'discord', label: 'Discord', available: true, offered: true }), + entry({ installableId: 'groupme', label: 'GroupMe', available: true, offered: true }), entry({ installableId: 'slack', label: 'Slack', available: true, offered: true }), ]); renderPage(); - const discord = await openPopover('Discord'); - expect(discord.textContent).not.toContain('one click in your workspace'); - expect(discord.textContent).not.toContain('one message'); - expect(within(discord).getByRole('button', { name: 'Add' })).toBeInTheDocument(); + const groupme = await openPopover('GroupMe'); + expect(groupme.textContent).not.toContain('one click in your workspace'); + expect(groupme.textContent).not.toContain('one message'); + expect(within(groupme).getByRole('button', { name: 'Add' })).toBeInTheDocument(); const slack = await openPopover('Slack'); expect(slack.textContent).toContain('one click in your workspace'); @@ -1210,19 +1210,20 @@ describe('Connectors page: channels', () => { }); it('does not list a provider this instance does not offer (D215)', async () => { - // Discord on commonly.me: usable keys, no builtin Installable row, never - // walked. A stranger must not see it, not even behind "+N off". + // Usable keys, no builtin Installable row, and no install link of its own. + // A stranger must not see it, not even behind "+N off". (Discord was this + // example until it got a link install; see the Discord block below.) mockCatalog([ entry(), - entry({ installableId: 'discord', label: 'Discord', available: true, offered: false }), + entry({ installableId: 'groupme', label: 'GroupMe', available: true, offered: false }), ]); renderPage(); await findTile('Telegram'); // The off button only exists when a dead tile does, so its absence is the - // assertion; a collapsed group would hide a listed Discord from the query. + // assertion; a collapsed group would hide a listed GroupMe from the query. expect(screen.queryByRole('button', { name: /^\+\d+ off$/ })).toBeNull(); - expect(tileNode('Discord')).toBeNull(); + expect(tileNode('GroupMe')).toBeNull(); }); it('keeps an installed but no-longer-offered provider as not connectable, with no Add anywhere', async () => { @@ -1230,17 +1231,17 @@ describe('Connectors page: channels', () => { // the roster row went away: it states the state and offers no action. Before // #1826 the row rendered an Add whose click ended in 404 installable_not_found. mockCatalog([ - entry({ installableId: 'discord', label: 'Discord', available: true, offered: false, installation: { status: 'active', boundPodId: 'p1' } }), + entry({ installableId: 'groupme', label: 'GroupMe', available: true, offered: false, installation: { status: 'active', boundPodId: 'p1' } }), ]); renderPage(); await pageReady(); await expandOff('Channels'); - const tile = await findTile('Discord'); + const tile = await findTile('GroupMe'); expect(tile).toHaveClass('v2-cx-tile--dim'); expect(tile.querySelector('svg')).not.toBeNull(); fireEvent.click(tile); - const drawer = await findDrawer('Discord'); + const drawer = await findDrawer('GroupMe'); expect(within(drawer).getByText('Unavailable here.')).toBeInTheDocument(); // No Add and no connect verb: an offered provider is the only thing that may // produce them, and there is none in this catalog. @@ -1255,15 +1256,96 @@ describe('Connectors page: channels', () => { // went away can leave an integration with no installation. Dropping it from // the list would hide a live connection from its owner. mockCatalog([ - entry({ installableId: 'discord', label: 'Discord', available: true, offered: false, integration: liveIntegration() }), + entry({ installableId: 'groupme', label: 'GroupMe', available: true, offered: false, integration: liveIntegration() }), ]); renderPage(); await pageReady(); await expandOff('Channels'); - expect(await findTile('Discord')).toHaveClass('v2-cx-tile--dim'); + expect(await findTile('GroupMe')).toHaveClass('v2-cx-tile--dim'); }); + describe('Discord: a link install, offered on its keys alone', () => { + // Discord has no builtin Installable row (the catalog says offered: false), + // but its install is the bot-add link plus Discord's callback, so it is + // offered wherever the instance holds its keys (available). Connect sends + // the browser to the link the server builds for the chosen pod; it never + // calls the Installable lifecycle, which would 422 provider_not_offered. + const realLocation = window.location; + let assign; + beforeEach(() => { + assign = jest.fn(); + Object.defineProperty(window, 'location', { configurable: true, value: { ...realLocation, assign } }); + }); + afterEach(() => { + Object.defineProperty(window, 'location', { configurable: true, value: realLocation }); + }); + + const discordEntry = entry({ installableId: 'discord', label: 'Discord', description: '', available: true, offered: false }); + const INSTALL_URL = 'https://discord.com/api/oauth2/authorize?client_id=1&response_type=code'; + + it('lists Discord with Add and its own onboarding line', async () => { + mockCatalog([entry(), discordEntry]); + renderPage(); + + const popover = await openPopover('Discord'); + expect(popover.textContent).toContain('add the bot to your server'); + expect(popover.textContent).not.toContain('Unavailable here.'); + expect(within(popover).getByRole('button', { name: 'Add' })).toBeInTheDocument(); + }); + + it('offers Discord beside Telegram and Slack in Connect a channel, and Connect leaves for its install link', async () => { + mockApi({ + installables: [entry(), entry({ installableId: 'slack', label: 'Slack', offered: true }), discordEntry], + pods: [{ _id: 'p1', name: 'Rewire Live Demo', type: 'chat', members: [] }], + route: (url) => (url === '/api/discord/install-link/p1' + ? Promise.resolve({ data: { installUrl: INSTALL_URL, podId: 'p1', status: 'ready' } }) + : undefined), + }); + renderPage(); + + fireEvent.click(await findTile('Channel')); + const drawer = await findDrawer('Connect a channel'); + const providers = within(drawer).getByRole('group', { name: 'Channel provider' }); + expect(within(providers).getAllByRole('button').map((b) => b.textContent)).toEqual(['Telegram', 'Slack', 'Discord']); + fireEvent.click(within(providers).getByRole('button', { name: 'Discord' })); + fireEvent.click(within(drawer).getByRole('button', { name: 'Connect' })); + + await waitFor(() => expect(assign).toHaveBeenCalledWith(INSTALL_URL)); + expect(axios.get).toHaveBeenCalledWith('/api/discord/install-link/p1', expect.anything()); + expect(axios.post).not.toHaveBeenCalledWith('/api/installables/discord/install', expect.anything(), expect.anything()); + }); + + it('does not list Discord on an instance without its keys', async () => { + mockCatalog([entry(), entry({ installableId: 'discord', label: 'Discord', available: false, offered: false })]); + renderPage(); + + await findTile('Telegram'); + expect(screen.queryByRole('button', { name: /^\+\d+ off$/ })).toBeNull(); + expect(tileNode('Discord')).toBeNull(); + }); + + it('says who may add the bot when the server refuses the link', async () => { + mockApi({ + installables: [discordEntry], + pods: [{ _id: 'p1', name: 'Rewire Live Demo', type: 'chat', members: [] }], + route: (url) => (url === '/api/discord/install-link/p1' + ? Promise.reject(Object.assign(new Error('403'), { response: { status: 403, data: { error: 'Access denied' } } })) + : undefined), + }); + renderPage(); + + const popover = await openPopover('Discord'); + fireEvent.click(within(popover).getByRole('button', { name: 'Add' })); + const drawer = await findDrawer('Connect a channel'); + fireEvent.click(within(drawer).getByRole('button', { name: 'Connect' })); + + expect(await screen.findByText('Only the person who created this pod can add the Discord bot to it.')).toBeInTheDocument(); + expect(assign).not.toHaveBeenCalled(); + }); + }); + + it('renders an unavailable provider with its blocker and an available one with Add', async () => { mockCatalog([ entry(), diff --git a/frontend/src/v2/connectors/ConnectorsPage.tsx b/frontend/src/v2/connectors/ConnectorsPage.tsx index 4a2f9ac408..4febb992bf 100644 --- a/frontend/src/v2/connectors/ConnectorsPage.tsx +++ b/frontend/src/v2/connectors/ConnectorsPage.tsx @@ -7,7 +7,7 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { useTranslation } from 'react-i18next'; import { useNavigate } from 'react-router-dom'; import { catalogDescription } from '../utils/connectorCatalogDescription'; -import { ADD_PLATFORMS, TYPE_LABELS } from './channelModel'; +import { ADD_PLATFORMS, TYPE_LABELS, entryOffered } from './channelModel'; import type { ListItem } from './channelModel'; import { PER_ENTRY_REVOKE_COPY, entryCovers, entryIsGranted, isDead } from './toolModel'; import type { ToolCatalogEntry, ToolGrant } from './toolModel'; @@ -89,7 +89,7 @@ const ConnectorsPage: React.FC = () => { (connector) => connector.type === type && Boolean(connector.installationId), ); const availableProviders = c.catalog - ? c.catalog.filter((entry) => entry.available && entry.offered !== false && !entry.installation).map((entry) => ({ type: entry.installableId })) + ? c.catalog.filter((entry) => entry.available && entryOffered(entry) && !entry.installation).map((entry) => ({ type: entry.installableId })) : ADD_PLATFORMS.filter((provider) => provider.enabled && !hasInstallation(provider.type)); // ---- tools ---- diff --git a/frontend/src/v2/connectors/channelModel.ts b/frontend/src/v2/connectors/channelModel.ts index 8410922bc8..dbc2b7519d 100644 --- a/frontend/src/v2/connectors/channelModel.ts +++ b/frontend/src/v2/connectors/channelModel.ts @@ -175,8 +175,24 @@ export const mappedTypeLabel = (type: string): string | null => TYPE_LABELS[type export const CONNECT_DETAIL: Record = { telegram: { key: 'connectors.availableTelegram', defaultValue: 'one message' }, slack: { key: 'connectors.availableSlack', defaultValue: 'one click in your workspace' }, + discord: { key: 'connectors.availableDiscord', defaultValue: 'add the bot to your server' }, }; +// Providers whose Connect sends the person to the provider's own install page +// instead of the Installable lifecycle. No builtin row is installed for them, so +// the catalog reports `offered: false`, but the install link and the provider's +// callback are the whole flow and `available` already says this instance holds +// their keys. Discord: the bot add returns to /api/discord/callback, and the +// success page binds the channel. +export const LINK_INSTALL_PROVIDERS = new Set(['discord']); + +// Offered for a channel picker: a rostered provider, or a link-install one +// whose keys this instance holds. Without the keys a link-install provider stays +// unlisted, exactly like an unrostered one. +export const entryOffered = (entry: { installableId: string; offered?: boolean; available?: boolean }): boolean => ( + entry.offered !== false || (entry.available === true && LINK_INSTALL_PROVIDERS.has(entry.installableId)) +); + // Fallback only: when the catalog cannot be read the page still offers the // two providers it can drive, exactly as it did before the catalog existed. export const ADD_PLATFORMS = [ diff --git a/frontend/src/v2/connectors/useChannelConnectors.ts b/frontend/src/v2/connectors/useChannelConnectors.ts index 5cada5161a..a6796b04cf 100644 --- a/frontend/src/v2/connectors/useChannelConnectors.ts +++ b/frontend/src/v2/connectors/useChannelConnectors.ts @@ -27,6 +27,8 @@ import { codeExpiresInMinutes, claimIsStale, projectionMissing, + LINK_INSTALL_PROVIDERS, + entryOffered, } from './channelModel'; import type { ConnectorGate, @@ -378,11 +380,37 @@ export const useChannelConnectors = () => { } }; + // A link-install provider's Connect: the server builds the provider's install + // URL for this pod, and the browser leaves for it. The provider sends the + // person back to its callback, which lands on the success page that binds the + // channel. Only the pod's creator (or an admin) may add a bot to a pod. + const linkInstall = async (type: string, podId: string): Promise => { + try { + const result = await api.get<{ installUrl?: string }>(`/api/${type}/install-link/${encodeURIComponent(podId)}`); + if (!result?.installUrl) throw new Error('no install link'); + window.location.assign(result.installUrl); + return true; + } catch (requestError) { + const response = installErrorResponse(requestError); + setError(response.status === 403 + ? t('connectors.linkInstallCreatorOnly', { + defaultValue: 'Only the person who created this pod can add the {{connector}} bot to it.', + connector: TYPE_LABELS[type] || type, + }) + : t('connectors.createError', { defaultValue: 'Could not create the connector.' })); + return false; + } + }; + const createConnector = async () => { if (!newPodId || !addingType || creating) return; setCreating(true); setError(null); try { + if (LINK_INSTALL_PROVIDERS.has(addingType)) { + await linkInstall(addingType, newPodId); + return; + } const installed = await install(addingType, newPodId); if (installed) setAddingType(null); } finally { @@ -765,7 +793,7 @@ export const useChannelConnectors = () => { // about this instance's keys, not about whether a row exists to install. // Showing the Add anyway is what produced a lowercase `discord` row whose // click ended in 404 installable_not_found. - if (entry.offered === false) { + if (!entryOffered(entry)) { return { action: null, detail: '', @@ -910,7 +938,7 @@ export const useChannelConnectors = () => { // offerability contract), and someone who already installed it keeps their // row, which renders in the not-connectable state below. const listedCatalog = (catalog || []).filter((entry) => ( - entry.offered !== false || Boolean(entry.installation) || Boolean(entry.integration) + entryOffered(entry) || Boolean(entry.installation) || Boolean(entry.integration) )); const allItems: ListItem[] = [ ...listedCatalog.map((entry): ListItem => ({ From f49ee4910d06efa43f4f5b27d7557360f3f1d333 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:56:53 -0800 Subject: [PATCH 2/3] fix(discord): the install state is signed and names the installer The install link's OAuth state was the bare string pod_. It is now a short-lived signed token naming the pod and the person who asked for the link, and the callback refuses anything else before the code exchange. The install proof the callback hands the success page carries that same person, and the channel list and the first bind accept it only from them. The legacy room built its own state client-side; it now asks the server for the link. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DT6tbhKm6UhsmFhLk9 --- .../service/discordInstallBind.test.js | 55 ++++++++++++++++++- .../routes/discordBindingContainment.test.js | 35 ++++++------ .../routes/discordChannelsRateLimit.test.js | 5 +- backend/routes/discord.ts | 16 ++++-- backend/routes/integrations.ts | 1 + backend/utils/discordInstallProof.ts | 55 +++++++++++++++++-- frontend/src/components/ChatRoom.tsx | 44 +++++++++------ 7 files changed, 167 insertions(+), 44 deletions(-) diff --git a/backend/__tests__/service/discordInstallBind.test.js b/backend/__tests__/service/discordInstallBind.test.js index dd1b4a3a29..3dd1b65c7f 100644 --- a/backend/__tests__/service/discordInstallBind.test.js +++ b/backend/__tests__/service/discordInstallBind.test.js @@ -130,7 +130,9 @@ test('the install link sends Discord back to the callback with a code', async () expect(url.searchParams.get('client_id')).toBe('1111111111'); expect(url.searchParams.get('response_type')).toBe('code'); expect(url.searchParams.get('redirect_uri')).toBe(CALLBACK); - expect(url.searchParams.get('state')).toBe(`pod_${POD_ID}`); + // A signed state, not the bare pod id a crafted link could carry. + expect(url.searchParams.get('state')).not.toBe(`pod_${POD_ID}`); + expect(url.searchParams.get('state')).not.toContain(POD_ID); expect(url.searchParams.get('scope')).toBe('bot applications.commands'); }); @@ -202,3 +204,54 @@ test('a pod member who did not create the pod gets no install link', async () => const res = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer member-token'); expect(res.status).toBe(403); }); + +// The callback has no session, so the state is the only thing tying a code to +// the person who asked for the install. Unsigned, a crafted link carrying an +// attacker's code and a victim's pod would mint a proof the victim's browser then +// spends binding their pod to the attacker's guild (CodeRabbit on this change). +describe('the install state and proof are bound to the person who asked', () => { + const callback = (state) => request(app).get(`/api/discord/callback?code=the-code&state=${encodeURIComponent(state)}&guild_id=${GUILD}`); + const errorOf = (res) => new URL(res.headers.location).pathname; + + test('a bare pod_ state is refused before the code is exchanged', async () => { + const res = await callback(`pod_${POD_ID}`); + expect(res.status).toBe(302); + expect(errorOf(res)).toBe('/discord/error'); + expect(axios.post.mock.calls.find(([url]) => url === 'https://discord.com/api/oauth2/token')).toBeUndefined(); + }); + + test('an expired state is refused', async () => { + const jwt = require('jsonwebtoken'); + const stale = jwt.sign({ pur: 'discord-install-state', pod: POD_ID, uid: OWNER }, process.env.JWT_SECRET, { expiresIn: -10 }); + const res = await callback(stale); + expect(errorOf(res)).toBe('/discord/error'); + }); + + test("a proof minted for one person is refused for another member of the same pod", async () => { + const link = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer owner-token'); + const state = new URL(link.body.installUrl).searchParams.get('state'); + const cb = await callback(state); + const proof = new URL(cb.headers.location).searchParams.get('install'); + expect(proof).toBeTruthy(); + + const channels = await request(app) + .get(`/api/discord/channels/${GUILD}?install=${encodeURIComponent(proof)}`) + .set('Authorization', 'Bearer member-token'); + expect(channels.status).toBe(404); + + const bind = await request(app) + .post('/api/integrations') + .set('Authorization', 'Bearer member-token') + .send({ + podId: POD_ID, + type: 'discord', + config: { + serverId: GUILD, serverName: 'Commonly Dev', channelId: CHANNEL, channelName: 'general', webhookUrl: '', botToken: '', + }, + install: proof, + }); + expect(bind.status).toBe(404); + expect(await Integration.findOne({ podId: POD_ID, type: 'discord' })).toBeNull(); + }); +}); + diff --git a/backend/__tests__/unit/routes/discordBindingContainment.test.js b/backend/__tests__/unit/routes/discordBindingContainment.test.js index ae9e4a74ed..43f7e8f617 100644 --- a/backend/__tests__/unit/routes/discordBindingContainment.test.js +++ b/backend/__tests__/unit/routes/discordBindingContainment.test.js @@ -292,7 +292,7 @@ describe('Discord binding containment (TASK-123 a)', () => { }; it('lists channels for a member of the pod the proof names', async () => { - const res = await firstInstall(mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD })); + const res = await firstInstall(mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD, userId: MEMBER })); expect(res.status).toBe(200); expect(res.body).toEqual([{ id: CHANNEL, name: 'general', topic: undefined }]); @@ -306,9 +306,9 @@ describe('Discord binding containment (TASK-123 a)', () => { }); it.each([ - ['a proof for another guild', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: OTHER_GUILD })], - ['a proof for a pod the caller is not in', () => mintDiscordInstallProof({ podId: 'pod-9', guildId: GUILD })], - ['an expired proof', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD }, -60)], + ['a proof for another guild', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: OTHER_GUILD, userId: MEMBER })], + ['a proof for a pod the caller is not in', () => mintDiscordInstallProof({ podId: 'pod-9', guildId: GUILD, userId: MEMBER })], + ['an expired proof', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD, userId: MEMBER }, -60)], ['a token that is not a proof', () => require('jsonwebtoken').sign({ id: MEMBER }, process.env.JWT_SECRET)], ['a forged proof', () => require('jsonwebtoken').sign({ pur: 'discord-install', pod: 'pod-1', gid: GUILD }, 'not-the-secret')], ])('refuses %s, without an outbound call', async (_label, makeProof) => { @@ -319,16 +319,19 @@ describe('Discord binding containment (TASK-123 a)', () => { }); it('does not authenticate a request on its own', async () => { - const proof = mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD }); + const proof = mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD, userId: MEMBER }); expect(require('jsonwebtoken').decode(proof)).not.toHaveProperty('id'); expect(require('jsonwebtoken').decode(proof)).not.toHaveProperty('user'); }); }); describe('GET /api/discord/callback', () => { + // install-link's signed state for MEMBER and pod-1 (a bare pod_ is refused). + const STATE = () => encodeURIComponent(require('../../../utils/discordInstallProof') + .mintDiscordInstallState({ podId: 'pod-1', userId: MEMBER })); it('refuses a malformed guild_id before the code exchange', async () => { const res = await request(app) - .get('/api/discord/callback?code=abc&state=pod_pod-1&guild_id=guild-1'); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=guild-1`); expect(res.status).toBe(302); expect(res.headers.location).toContain('/discord/error'); @@ -338,7 +341,7 @@ describe('Discord binding containment (TASK-123 a)', () => { it('still completes the exchange for a well-formed guild_id', async () => { const res = await request(app) - .get(`/api/discord/callback?code=abc&state=pod_pod-1&guild_id=${GUILD}`); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=${GUILD}`); expect(res.status).toBe(302); expect(res.headers.location).toContain('/discord/success'); @@ -350,19 +353,19 @@ describe('Discord binding containment (TASK-123 a)', () => { const { readDiscordInstallProof } = require('../../../utils/discordInstallProof'); const res = await request(app) - .get(`/api/discord/callback?code=abc&state=pod_pod-1&guild_id=${GUILD}`); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=${GUILD}`); expect(res.status).toBe(302); const location = new URL(res.headers.location); expect(location.pathname).toBe('/discord/success'); - expect(readDiscordInstallProof(location.searchParams.get('install'))).toEqual({ podId: 'pod-1', guildId: GUILD }); + expect(readDiscordInstallProof(location.searchParams.get('install'))).toEqual({ podId: 'pod-1', guildId: GUILD, userId: MEMBER }); }); it('refuses a guild_id that differs from the guild in the token response', async () => { axios.post.mockResolvedValueOnce({ data: { token_type: 'Bearer', guild: { id: OTHER_GUILD } } }); const res = await request(app) - .get(`/api/discord/callback?code=abc&state=pod_pod-1&guild_id=${GUILD}`); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=${GUILD}`); expect(res.status).toBe(302); expect(res.headers.location).toContain('/discord/error'); @@ -371,7 +374,7 @@ describe('Discord binding containment (TASK-123 a)', () => { it('mints no proof when the token response names no guild', async () => { const res = await request(app) - .get(`/api/discord/callback?code=abc&state=pod_pod-1&guild_id=${GUILD}`); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=${GUILD}`); expect(res.status).toBe(302); expect(new URL(res.headers.location).searchParams.has('install')).toBe(false); @@ -384,7 +387,7 @@ describe('Discord binding containment (TASK-123 a)', () => { const errorLog = jest.spyOn(console, 'error').mockImplementation(() => {}); try { const res = await request(app) - .get(`/api/discord/callback?code=abc&state=pod_pod-1&guild_id=${GUILD}`); + .get(`/api/discord/callback?code=abc&state=${STATE()}&guild_id=${GUILD}`); expect(axios.post).toHaveBeenCalledWith( 'https://discord.com/api/oauth2/token', expect.anything(), expect.anything(), ); @@ -613,7 +616,7 @@ describe('Discord binding containment (TASK-123 a)', () => { it('accepts a first install on the callback’s proof for this pod and guild, and does not store it', async () => { Integration.findOne.mockResolvedValue(null); - const install = mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD }); + const install = mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD, userId: MEMBER }); const res = await post({ ...BODY, install }); @@ -622,9 +625,9 @@ describe('Discord binding containment (TASK-123 a)', () => { }); it.each([ - ['a proof for another pod', () => mintDiscordInstallProof({ podId: 'pod-2', guildId: GUILD })], - ['a proof for another guild', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: OTHER_GUILD })], - ['an expired proof', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD }, -10)], + ['a proof for another pod', () => mintDiscordInstallProof({ podId: 'pod-2', guildId: GUILD, userId: MEMBER })], + ['a proof for another guild', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: OTHER_GUILD, userId: MEMBER })], + ['an expired proof', () => mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD, userId: MEMBER }, -10)], ['a session token in the proof field', () => 'user-token'], ])('refuses %s', async (_label, make) => { Integration.findOne.mockResolvedValue(null); diff --git a/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js b/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js index 70767a6565..337bbb1654 100644 --- a/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js +++ b/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js @@ -107,7 +107,10 @@ it('redirects a failed Discord OAuth exchange even without FRONTEND_URL', async axios.post.mockRejectedValueOnce(new Error('exchange failed')); const log = jest.spyOn(console, 'error').mockImplementation(() => {}); try { - const res = await request(app).get('/api/discord/callback?code=oauth-code&state=pod_123'); + process.env.JWT_SECRET = process.env.JWT_SECRET || 'test-secret'; + const state = encodeURIComponent(require('../../../utils/discordInstallProof') + .mintDiscordInstallState({ podId: '123', userId: 'user-1' })); + const res = await request(app).get(`/api/discord/callback?code=oauth-code&state=${state}`); expect(res.status).toBe(302); expect(res.headers.location).toBe('http://localhost:3000/discord/error?error=OAuth%20authorization%20failed'); } finally { diff --git a/backend/routes/discord.ts b/backend/routes/discord.ts index a2498e6f34..707b81841e 100644 --- a/backend/routes/discord.ts +++ b/backend/routes/discord.ts @@ -25,7 +25,9 @@ const { runDiscordCommandForIntegrations } = require('../services/discordMultiCo import { listIntegrationsRateLimit, writeIntegrationsRateLimit } from '../middleware/integrationRateLimit'; import { platformIpRateLimit } from '../middleware/platformRateLimit'; import { invalidDiscordIdError, isDiscordSnowflake, isSupplied } from '../utils/discordBinding'; -import { mintDiscordInstallProof, readDiscordInstallProof } from '../utils/discordInstallProof'; +import { + mintDiscordInstallProof, mintDiscordInstallState, readDiscordInstallProof, readDiscordInstallState, +} from '../utils/discordInstallProof'; interface AuthReq { user?: { id: string; role?: string }; @@ -131,6 +133,7 @@ router.get('/channels/:guildId', listIntegrationsRateLimit, auth, async (req: Au // stands in for one: same guild, and a pod the caller belongs to (F89). const proof = binding ? null : readDiscordInstallProof(req.query?.install); const provenInstall = !!proof && proof.guildId === guildId + && proof.userId === String(req.user?.id) && podIds.some((podId: unknown) => String(podId) === proof.podId); // One answer for a guild that is bound elsewhere and one that does not // exist: the route must not become an oracle for either. @@ -235,7 +238,7 @@ router.get('/install-link/:podId', auth, async (req: AuthReq, res: Res) => { installUrl.searchParams.set('permissions', DISCORD_BOT_PERMISSIONS); installUrl.searchParams.set('response_type', 'code'); installUrl.searchParams.set('redirect_uri', discordRedirectUri()); - installUrl.searchParams.set('state', `pod_${podId}`); + installUrl.searchParams.set('state', mintDiscordInstallState({ podId: String(podId), userId: String(req.user?.id) })); res.json({ installUrl: installUrl.toString(), podId, status: 'ready' }); } catch { console.error('Error generating installation link'); @@ -348,8 +351,11 @@ router.get('/callback', discordCallbackLimit, async (req: AuthReq, res: Res) => try { const { code, state, guild_id: guildId } = req.query || {}; if (!code) return res.redirect(`${frontend}/discord/error?error=No authorization code received`); - const podId = state?.replace('pod_', ''); - if (!podId) return res.redirect(`${frontend}/discord/error?error=Invalid state parameter`); + // A signed state from install-link, never a bare pod id (see + // utils/discordInstallProof.ts). Refused before the code is exchanged. + const installState = readDiscordInstallState(state); + if (!installState) return res.redirect(`${frontend}/discord/error?error=Invalid state parameter`); + const { podId, userId: installerId } = installState; // Unauthenticated route, so `guild_id` here is request-tainted input on a URL // built with the instance bot token. Refused before the code exchange: a // malformed id must not reach Discord at all, and the same value is forwarded @@ -383,7 +389,7 @@ router.get('/callback', discordCallbackLimit, async (req: AuthReq, res: Res) => successUrl.searchParams.append('pod_id', podId); successUrl.searchParams.append('guild_id', installedGuildId || ''); successUrl.searchParams.append('server_name', serverName); - if (provenGuildId) successUrl.searchParams.append('install', mintDiscordInstallProof({ podId, guildId: provenGuildId })); + if (provenGuildId) successUrl.searchParams.append('install', mintDiscordInstallProof({ podId, guildId: provenGuildId, userId: installerId })); res.redirect(successUrl.toString()); } catch { console.error('Discord OAuth callback error'); diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index 764f9edde8..b3702f9296 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -87,6 +87,7 @@ const hasDiscordGuildAuthority = async ( })) return true; const proof = readDiscordInstallProof(install); return !!proof && proof.guildId === guildId && proof.podId === podId + && proof.userId === String(userId) && podIds.some((id: unknown) => String(id) === podId); }; diff --git a/backend/utils/discordInstallProof.ts b/backend/utils/discordInstallProof.ts index 8a8b1351b0..7c5e69f354 100644 --- a/backend/utils/discordInstallProof.ts +++ b/backend/utils/discordInstallProof.ts @@ -24,6 +24,8 @@ interface DiscordInstallProofPayload extends JwtPayload { pur: typeof PURPOSE; pod: string; gid: string; + // The person whose install this is: the state's user. Only they may spend it. + uid: string; } const secret = (): string => { @@ -33,20 +35,63 @@ const secret = (): string => { }; export const mintDiscordInstallProof = ( - { podId, guildId }: { podId: string; guildId: string }, + { podId, guildId, userId }: { podId: string; guildId: string; userId: string }, ttlSeconds: number = DISCORD_INSTALL_PROOF_TTL_SECONDS, ): string => { - const payload: DiscordInstallProofPayload = { pur: PURPOSE, pod: podId, gid: guildId }; + const payload: DiscordInstallProofPayload = { pur: PURPOSE, pod: podId, gid: guildId, uid: userId }; return jwt.sign(payload, secret(), { expiresIn: ttlSeconds }); }; -/** The proof's (pod, guild), or null for anything that is not a live proof. */ -export const readDiscordInstallProof = (token: unknown): { podId: string; guildId: string } | null => { +/** + * The proof's (pod, guild, user), or null for anything that is not a live proof. + * A proof without a user (minted before the state was signed) is not a proof. + */ +export const readDiscordInstallProof = (token: unknown): { podId: string; guildId: string; userId: string } | null => { if (typeof token !== 'string' || !token) return null; try { const decoded = jwt.verify(token, secret()) as DiscordInstallProofPayload; if (decoded.pur !== PURPOSE || typeof decoded.pod !== 'string' || !isDiscordSnowflake(decoded.gid)) return null; - return { podId: decoded.pod, guildId: decoded.gid }; + if (typeof decoded.uid !== 'string' || !decoded.uid) return null; + return { podId: decoded.pod, guildId: decoded.gid, userId: decoded.uid }; + } catch { + return null; + } +}; + +// ---- The install state ---- +// The OAuth `state` the install link sends Discord, and Discord hands back to the +// callback. The callback has no session, so this is the only thing that ties a +// code to the person who asked for the install. It used to be the bare string +// `pod_`: a crafted callback link carrying an attacker's code and a victim's +// pod minted a proof that the victim's browser then spent binding their pod to +// the attacker's guild. Signed, short-lived and naming the user, it can only be +// obtained from install-link by someone allowed to install into that pod, and the +// proof it leads to can be spent only by that same person. +const STATE_PURPOSE = 'discord-install-state'; +export const DISCORD_INSTALL_STATE_TTL_SECONDS = 15 * 60; + +interface DiscordInstallStatePayload extends JwtPayload { + pur: typeof STATE_PURPOSE; + pod: string; + uid: string; +} + +export const mintDiscordInstallState = ( + { podId, userId }: { podId: string; userId: string }, + ttlSeconds: number = DISCORD_INSTALL_STATE_TTL_SECONDS, +): string => { + const payload: DiscordInstallStatePayload = { pur: STATE_PURPOSE, pod: podId, uid: userId }; + return jwt.sign(payload, secret(), { expiresIn: ttlSeconds }); +}; + +/** The state's (pod, user), or null for anything that is not a live state. */ +export const readDiscordInstallState = (token: unknown): { podId: string; userId: string } | null => { + if (typeof token !== 'string' || !token) return null; + try { + const decoded = jwt.verify(token, secret()) as DiscordInstallStatePayload; + if (decoded.pur !== STATE_PURPOSE || typeof decoded.pod !== 'string' || !decoded.pod) return null; + if (typeof decoded.uid !== 'string' || !decoded.uid) return null; + return { podId: decoded.pod, userId: decoded.uid }; } catch { return null; } diff --git a/frontend/src/components/ChatRoom.tsx b/frontend/src/components/ChatRoom.tsx index 2c3fe9e140..2a838be28d 100644 --- a/frontend/src/components/ChatRoom.tsx +++ b/frontend/src/components/ChatRoom.tsx @@ -1145,24 +1145,29 @@ const ChatRoom = () => { } }; - const getDiscordOAuthUrl = (podId, guildId = null) => { - const clientId = process.env.REACT_APP_DISCORD_CLIENT_ID; - const redirectUri = encodeURIComponent(`${getApiBaseUrl()}/api/discord/callback`); - const scopes = encodeURIComponent('bot applications.commands'); - const permissions = '536873984'; - const state = `pod_${podId}`; - const timestamp = Date.now(); - return `https://discord.com/api/oauth2/authorize?client_id=${clientId}&scope=${scopes}&permissions=${permissions}` - + `&redirect_uri=${redirectUri}&response_type=code&state=${state}` - + (guildId ? `&guild_id=${guildId}` : '') - + `&t=${timestamp}`; + // The install link comes from the server: its OAuth state is signed and names + // the person asking, and the callback refuses any other state. Opened in a tab + // synchronously (so the popup blocker allows it), then pointed at the link. + const openDiscordInstall = async (guildId = null) => { + if (!roomId) return; + const tab = window.open('', '_blank'); + try { + const token = localStorage.getItem('token'); + const res = await axios.get(`/api/discord/install-link/${roomId}`, { + headers: { Authorization: `Bearer ${token}` }, + }); + const url = new URL(res.data.installUrl); + if (guildId) url.searchParams.set('guild_id', guildId); + if (tab) tab.location.href = url.toString(); + else window.location.assign(url.toString()); + } catch (err) { + if (tab) tab.close(); + console.warn('Failed to get the Discord install link:', err.response?.status); + } }; const getIntegrationRedirectUrl = (type) => { if (!roomId) return '#'; - if (type === 'discord') { - return getDiscordOAuthUrl(roomId); - } const externalSetupLinks = { slack: 'https://api.slack.com/apps', groupme: 'https://dev.groupme.com/bots/new', @@ -1731,7 +1736,7 @@ const ChatRoom = () => { if (config.serverId && config.channelId) { return `https://discord.com/channels/${config.serverId}/${config.channelId}`; } - return getDiscordOAuthUrl(roomId, config.serverId); + return null; case 'slack': return 'https://api.slack.com/apps'; case 'groupme': @@ -1764,6 +1769,10 @@ const ChatRoom = () => { return; } const url = getIntegrationManageUrl(type, integration); + if (!url && type === 'discord') { + openDiscordInstall(integration?.config?.serverId || null); + return; + } if (url) { window.open(url, '_blank', 'noopener,noreferrer'); } @@ -3626,7 +3635,10 @@ const ChatRoom = () => { target={['groupme', 'telegram', 'x', 'instagram'].includes(item.id) ? undefined : '_blank'} rel={['groupme', 'telegram', 'x', 'instagram'].includes(item.id) ? undefined : 'noopener noreferrer'} onClick={(event) => { - if (item.id === 'groupme') { + if (item.id === 'discord') { + event.preventDefault(); + openDiscordInstall(); + } else if (item.id === 'groupme') { event.preventDefault(); handleGroupmeSetupOpen(); } else if (item.id === 'telegram') { From c5b69b30e8efcb275c8956827fa733486cf7bc1e Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:04:09 -0800 Subject: [PATCH 3/3] fix(discord): binding a channel takes the pod creator or an admin A channel bridge is a pod-level setting, so binding one now takes the same authority as the install link: the pod's creator or an instance admin. Authority over the guild alone (a binding the caller made elsewhere) no longer lets a member bridge a pod they did not create. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DT6tbhKm6UhsmFhLk9 --- .../service/discordInstallBind.test.js | 69 ++++++++++++++++++- .../integrations.discordTokenCopy.test.js | 4 +- backend/routes/integrations.ts | 9 +++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/service/discordInstallBind.test.js b/backend/__tests__/service/discordInstallBind.test.js index 3dd1b65c7f..7960928c92 100644 --- a/backend/__tests__/service/discordInstallBind.test.js +++ b/backend/__tests__/service/discordInstallBind.test.js @@ -250,8 +250,75 @@ describe('the install state and proof are bound to the person who asked', () => }, install: proof, }); - expect(bind.status).toBe(404); + // A member is refused at the pod check (only the creator or an admin binds). + expect(bind.status).toBe(403); + expect(await Integration.findOne({ podId: POD_ID, type: 'discord' })).toBeNull(); + + // An admin passes the pod check, and is still refused another person's proof. + User.findById.mockImplementation(async (id) => ({ _id: String(id), role: String(id) === MEMBER ? 'admin' : 'user' })); + const adminBind = await request(app) + .post('/api/integrations') + .set('Authorization', 'Bearer member-token') + .send({ + podId: POD_ID, + type: 'discord', + config: { + serverId: GUILD, serverName: 'Commonly Dev', channelId: CHANNEL, channelName: 'general', webhookUrl: '', botToken: '', + }, + install: proof, + }); + expect(adminBind.status).toBe(404); expect(await Integration.findOne({ podId: POD_ID, type: 'discord' })).toBeNull(); }); }); + +// A channel bridge is a pod-level setting, so binding one takes the same +// authority as the install link: the pod's creator or an instance admin. The +// "you bound this guild before" arm proves authority over the GUILD, not over +// the pod, so on its own it let a plain member bridge someone else's pod. +describe('binding a Discord channel takes authority over the pod', () => { + const MEMBERS_OWN_POD = '6a8f6de2a1dccf2e02f314ff'; + const bindAs = (token, install) => request(app) + .post('/api/integrations') + .set('Authorization', `Bearer ${token}`) + .send({ + podId: POD_ID, + type: 'discord', + config: { + serverId: GUILD, serverName: 'Commonly Dev', channelId: CHANNEL, channelName: 'general', webhookUrl: '', botToken: '', + }, + ...(install ? { install } : {}), + }); + + test("a member who bound the guild to their own pod still can't bridge a pod they didn't create", async () => { + Pod.find.mockImplementation((query) => { + const userId = String(query?.$or?.[0]?.members || ''); + const rows = userId === MEMBER ? [{ _id: POD_ID }, { _id: MEMBERS_OWN_POD }] : [{ _id: POD_ID }]; + return { select: () => ({ lean: async () => rows }) }; + }); + await Integration.collection.insertOne({ + podId: new mongoose.Types.ObjectId(MEMBERS_OWN_POD), + type: 'discord', + status: 'connected', + config: { serverId: GUILD, channelId: '777777777777777777' }, + createdBy: new mongoose.Types.ObjectId(MEMBER), + }); + + const res = await bindAs('member-token'); + expect(res.status).toBe(403); + expect(await Integration.findOne({ podId: POD_ID, type: 'discord' })).toBeNull(); + expect(axios.post.mock.calls.find(([url]) => url.endsWith('/webhooks'))).toBeUndefined(); + }); + + test('an instance admin may bind a pod they did not create, on a proof minted for them', async () => { + User.findById.mockImplementation(async (id) => ({ _id: String(id), role: String(id) === MEMBER ? 'admin' : 'user' })); + const link = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer member-token'); + expect(link.status).toBe(200); + const state = new URL(link.body.installUrl).searchParams.get('state'); + const cb = await request(app).get(`/api/discord/callback?code=the-code&state=${encodeURIComponent(state)}&guild_id=${GUILD}`); + const proof = new URL(cb.headers.location).searchParams.get('install'); + + expect((await bindAs('member-token', proof)).status).toBe(201); + }); +}); diff --git a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js index 9e9c344205..303c92178f 100644 --- a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js +++ b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js @@ -167,7 +167,9 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { .set('x-test-user', String(joiner._id)) .send({ podId: String(pod._id), type: 'discord', config: { serverId: GUILD, channelId: CHANNEL } }); - expect(res.status).toBe(404); + // Refused at the pod check now (only the creator or an admin binds a + // channel); before that, at the guild check. Either way nothing is written. + expect(res.status).toBe(403); expect(axios.post).not.toHaveBeenCalled(); expect(await DiscordIntegration.countDocuments({ channelId: CHANNEL })).toBe(0); }); diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index b3702f9296..0293b86269 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -558,6 +558,15 @@ router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res if (type === 'discord') { // Authority, then the channel: both before the row or the webhook exist. // One answer for both refusals, as on the channel list. + // A channel bridge is a pod-level setting: binding one takes authority + // over the POD, the same as the install link (routes/discord.ts + // canManagePod: the pod's creator or an instance admin). Guild authority + // alone is not enough: the prior-binding arm below proves the guild, and + // on its own let a plain member bridge a pod they did not create. + const binder = await User.findById(String(req.user?.id)) as { role?: string } | null; + if (binder?.role !== 'admin' && targetPod.createdBy?.toString() !== String(req.user?.id)) { + return res.status(403).json({ message: 'Only the pod creator can connect Discord to this pod' }); + } const guildId = String(config.serverId); const install = (req.body as { install?: unknown })?.install; if (!await hasDiscordGuildAuthority(req.user?.id, String(podId), guildId, install)