From 436df12dd626984e52e588bec87023d3da2c3ce5 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:27:32 -0800 Subject: [PATCH 1/3] fix(discord): the first install's channel picker loads, through a proof the OAuth callback mints (F89) GET /api/discord/channels/:guildId answers only for a guild already bound to one of the caller's pods, so on a first install the success page's picker 404'd and no binding could be made. The callback now reads the guild from the token exchange's response (the query guild_id is only a hint and is refused when it disagrees) and hands the page a 30-minute signed proof of (pod, guild). The channels route accepts it in place of a binding when the caller belongs to that pod. No guild in the token response means no proof, which is today's behaviour. The guild lookup runs only when the exact value it uses is a snowflake (CodeQL 1925). Rebased onto main after #3360 replaced DiscordCallback with V2DiscordCallback: the forwarding and its two tests moved there. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- .../routes/discordBindingContainment.test.js | 80 +++++++++++++++++++ backend/routes/discord.ts | 26 ++++-- backend/utils/discordInstallProof.ts | 53 ++++++++++++ .../v2/__tests__/V2DiscordCallback.test.tsx | 25 ++++++ .../src/v2/components/V2DiscordCallback.tsx | 4 + 5 files changed, 183 insertions(+), 5 deletions(-) create mode 100644 backend/utils/discordInstallProof.ts diff --git a/backend/__tests__/unit/routes/discordBindingContainment.test.js b/backend/__tests__/unit/routes/discordBindingContainment.test.js index a5dbd6780..3cd3ca835 100644 --- a/backend/__tests__/unit/routes/discordBindingContainment.test.js +++ b/backend/__tests__/unit/routes/discordBindingContainment.test.js @@ -168,6 +168,7 @@ describe('Discord binding containment (TASK-123 a)', () => { process.env.DISCORD_BOT_TOKEN = 'bot-secret'; process.env.DISCORD_CLIENT_ID = 'client-id'; process.env.DISCORD_CLIENT_SECRET = 'client-secret'; + process.env.JWT_SECRET = 'test-jwt-secret'; // The create path encrypts the webhook URL (TASK-124 part 2), so a connector // secret ring is part of the deployment this route runs against. process.env.CONNECTOR_SECRET_KEYS = `k1:${Buffer.alloc(32, 1).toString('base64')}`; @@ -277,6 +278,53 @@ describe('Discord binding containment (TASK-123 a)', () => { }); }); + // F89: on a first install no binding exists yet, so the channel picker reads + // through the proof the OAuth callback minted. It is scoped to one pod and one + // guild, the guild taken from Discord's token response, and the caller must + // still belong to that pod. + describe('GET /api/discord/channels/:guildId on a first install', () => { + const { mintDiscordInstallProof } = require('../../../utils/discordInstallProof'); + const firstInstall = (proof) => { + Integration.findOne.mockResolvedValue(null); + return request(app) + .get(`/api/discord/channels/${GUILD}${proof === undefined ? '' : `?install=${encodeURIComponent(proof)}`}`) + .set('Authorization', 'Bearer user-token'); + }; + + it('lists channels for a member of the pod the proof names', async () => { + const res = await firstInstall(mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD })); + + expect(res.status).toBe(200); + expect(res.body).toEqual([{ id: CHANNEL, name: 'general', topic: undefined }]); + }); + + it('still answers 404 with no proof', async () => { + const res = await firstInstall(undefined); + + expect(res.status).toBe(404); + expect(outboundCalls()).toBe(0); + }); + + 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 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) => { + const res = await firstInstall(makeProof()); + + expect(res.status).toBe(404); + expect(outboundCalls()).toBe(0); + }); + + it('does not authenticate a request on its own', async () => { + const proof = mintDiscordInstallProof({ podId: 'pod-1', guildId: GUILD }); + expect(require('jsonwebtoken').decode(proof)).not.toHaveProperty('id'); + expect(require('jsonwebtoken').decode(proof)).not.toHaveProperty('user'); + }); + }); + describe('GET /api/discord/callback', () => { it('refuses a malformed guild_id before the code exchange', async () => { const res = await request(app) @@ -297,6 +345,38 @@ describe('Discord binding containment (TASK-123 a)', () => { expect(axios.post.mock.calls[0][0]).toBe('https://discord.com/api/oauth2/token'); }); + it('hands the success page a proof for the guild Discord names in the token response', async () => { + axios.post.mockResolvedValueOnce({ data: { token_type: 'Bearer', guild: { id: GUILD, name: 'Guild' } } }); + const { readDiscordInstallProof } = require('../../../utils/discordInstallProof'); + + const res = await request(app) + .get(`/api/discord/callback?code=abc&state=pod_pod-1&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 }); + }); + + 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}`); + + expect(res.status).toBe(302); + expect(res.headers.location).toContain('/discord/error'); + expect(axios.get).not.toHaveBeenCalled(); + }); + + 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}`); + + expect(res.status).toBe(302); + expect(new URL(res.headers.location).searchParams.has('install')).toBe(false); + }); + it('redirects a post-exchange guild failure without logging provider data', async () => { const sentinel = 'dummy-bot-token-sentinel'; axios.get.mockRejectedValueOnce({ response: { data: { detail: sentinel } } }); diff --git a/backend/routes/discord.ts b/backend/routes/discord.ts index 45424cf7d..cd041d3ed 100644 --- a/backend/routes/discord.ts +++ b/backend/routes/discord.ts @@ -25,6 +25,7 @@ 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'; interface AuthReq { user?: { id: string; role?: string }; @@ -116,9 +117,14 @@ router.get('/channels/:guildId', listIntegrationsRateLimit, auth, async (req: Au const binding = podIds.length ? await Integration.findOne({ type: 'discord', 'config.serverId': guildId, podId: { $in: podIds } }) : null; + // A first install has no binding yet. The proof the OAuth callback minted + // 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 + && 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. - if (!binding) return res.status(404).json({ message: 'Guild not found' }); + if (!binding && !provenInstall) return res.status(404).json({ message: 'Guild not found' }); const response = await axios.get(`https://discord.com/api/guilds/${guildId}/channels`, { headers: { Authorization: `Bot ${process.env.DISCORD_BOT_TOKEN}` } }); const textChannels = (response.data as Array<{ type: number; id: string; name: string; topic?: string }>) .filter((channel) => channel.type === 0) @@ -330,11 +336,20 @@ router.get('/callback', discordCallbackLimit, async (req: AuthReq, res: Res) => if (isSupplied(guildId) && !isDiscordSnowflake(guildId)) { return res.redirect(`${frontend}/discord/error?error=Invalid guild id`); } - 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: `${process.env.BACKEND_URL || 'http://localhost:5000'}/api/discord/callback` }, { 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). + const tokenGuildId = (tokenResponse?.data as { guild?: { id?: unknown } } | undefined)?.guild?.id; + const provenGuildId = isDiscordSnowflake(tokenGuildId) ? String(tokenGuildId) : null; + if (provenGuildId && isSupplied(guildId) && guildId !== provenGuildId) { + return res.redirect(`${frontend}/discord/error?error=OAuth%20authorization%20failed`); + } + const installedGuildId = provenGuildId || guildId; let serverName = 'Unknown Server'; - if (guildId) { + if (isDiscordSnowflake(installedGuildId)) { try { - const guildResponse = await axios.get(`https://discord.com/api/guilds/${guildId}`, { headers: { Authorization: `Bot ${process.env.DISCORD_BOT_TOKEN}` } }); + const guildResponse = await axios.get(`https://discord.com/api/guilds/${installedGuildId}`, { headers: { Authorization: `Bot ${process.env.DISCORD_BOT_TOKEN}` } }); serverName = (guildResponse.data as { name?: string }).name || serverName; } catch { // Discord may echo request credentials in its error body. A failed @@ -345,8 +360,9 @@ router.get('/callback', discordCallbackLimit, async (req: AuthReq, res: Res) => } const successUrl = new URL(`${frontend}/discord/success`); successUrl.searchParams.append('pod_id', podId); - successUrl.searchParams.append('guild_id', guildId || ''); + successUrl.searchParams.append('guild_id', installedGuildId || ''); successUrl.searchParams.append('server_name', serverName); + if (provenGuildId) successUrl.searchParams.append('install', mintDiscordInstallProof({ podId, guildId: provenGuildId })); res.redirect(successUrl.toString()); } catch { console.error('Discord OAuth callback error'); diff --git a/backend/utils/discordInstallProof.ts b/backend/utils/discordInstallProof.ts new file mode 100644 index 000000000..8a8b1351b --- /dev/null +++ b/backend/utils/discordInstallProof.ts @@ -0,0 +1,53 @@ +/** + * Discord first-install proof (F89). + * + * The success page's channel picker reads `GET /api/discord/channels/:guildId`, + * which answers only for a guild already bound to one of the caller's pods. On a + * first install nothing is bound yet, so the picker never loaded. The OAuth + * callback is where Commonly learns which guild the bot was just added to: the + * code exchange's token response names it. The callback has no session (Discord + * redirects a browser there), so it hands the page a short-lived signed proof of + * (pod, guild), and the channels route accepts that proof in place of a binding + * when the caller belongs to the pod it names. + * + * The payload carries neither `id` nor `user`, so `middleware/auth` never reads + * it as a session. + */ +import jwt, { type JwtPayload } from 'jsonwebtoken'; + +import { isDiscordSnowflake } from './discordBinding'; + +const PURPOSE = 'discord-install'; +export const DISCORD_INSTALL_PROOF_TTL_SECONDS = 30 * 60; + +interface DiscordInstallProofPayload extends JwtPayload { + pur: typeof PURPOSE; + pod: string; + gid: string; +} + +const secret = (): string => { + const value = process.env.JWT_SECRET; + if (!value) throw new Error('JWT_SECRET is not set'); + return value; +}; + +export const mintDiscordInstallProof = ( + { podId, guildId }: { podId: string; guildId: string }, + ttlSeconds: number = DISCORD_INSTALL_PROOF_TTL_SECONDS, +): string => { + const payload: DiscordInstallProofPayload = { pur: PURPOSE, pod: podId, gid: guildId }; + 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 => { + 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 }; + } catch { + return null; + } +}; diff --git a/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx b/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx index 8740df52b..0cae5a792 100644 --- a/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx +++ b/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx @@ -149,6 +149,31 @@ describe('V2DiscordCallback (d215 s-handoff)', () => { expect(route('error')?.trim()).toBe('type="error"'); }); + // F89: on a first install the channel list only answers on the callback's + // install proof, so the page must forward it, and send nothing extra without one. + test('forwards the install proof to the channel list', async () => { + // Production-shaped: the callback mints a three-part JWT, and the whole of it + // must arrive, not a value cut at a dot or a URL-decoding boundary. Joined at + // runtime so the secret scanner does not read a fixture as a token. + const proof = ['eyJhbGciOiJIUzI1NiJ9', 'eyJwdXIiOiJkaXNjb3JkLWluc3RhbGwifQ', 'c2lnbmF0dXJlLV8'].join('.'); + localStorage.setItem('token', 't'); + axios.get.mockResolvedValue({ data: [] }); + renderAt(`/discord/success?pod_id=pod-1&guild_id=123456789012345678&server_name=Guild&install=${encodeURIComponent(proof)}`); + await waitFor(() => expect(axios.get).toHaveBeenCalled()); + expect(axios.get).toHaveBeenCalledWith('/api/discord/channels/123456789012345678', { + headers: { Authorization: 'Bearer t' }, + params: { install: proof }, + }); + }); + + test('sends no params to the channel list without a proof', async () => { + localStorage.setItem('token', 't'); + axios.get.mockResolvedValue({ data: [] }); + renderAt('/discord/success?pod_id=pod-1&guild_id=123456789012345678&server_name=Guild'); + await waitFor(() => expect(axios.get).toHaveBeenCalled()); + expect(axios.get.mock.calls[0][1]).toEqual({ headers: { Authorization: 'Bearer t' } }); + }); + test('reads in Chinese under zh-CN', async () => { await act(async () => { await i18n.changeLanguage('zh-CN'); }); renderAt('/discord/error'); diff --git a/frontend/src/v2/components/V2DiscordCallback.tsx b/frontend/src/v2/components/V2DiscordCallback.tsx index e1cec7794..448f19d13 100644 --- a/frontend/src/v2/components/V2DiscordCallback.tsx +++ b/frontend/src/v2/components/V2DiscordCallback.tsx @@ -47,6 +47,9 @@ const V2DiscordCallback: React.FC = ({ type = 'callback' const guildId = urlParams.get('guild_id'); const serverName = urlParams.get('server_name'); const errorMessage = urlParams.get('error'); + // The callback's first-install proof: the channel list answers on it before a + // binding exists (F89). + const installProof = urlParams.get('install'); useEffect(() => { if (type === 'success' && guildId) { @@ -64,6 +67,7 @@ const V2DiscordCallback: React.FC = ({ type = 'callback' const token = localStorage.getItem('token'); const response = await axios.get(`/api/discord/channels/${id}`, { headers: { Authorization: `Bearer ${token}` }, + ...(installProof ? { params: { install: installProof } } : {}), }); setChannels(response.data); } catch (err) { From 77a4a8d180c783a82eabdbbda8147a56ae2cdab6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:57:03 -0800 Subject: [PATCH 2/3] fix(discord): a failed channel fetch logs its status, not the request carrying the install proof CodeRabbit on #3367: an axios error holds its request config, so logging it put params.install in the console. Reverse-checked: logging the error again fails the new test. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- .../src/v2/__tests__/V2DiscordCallback.test.tsx | 13 +++++++++++++ frontend/src/v2/components/V2DiscordCallback.tsx | 4 +++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx b/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx index 0cae5a792..eaa98bccd 100644 --- a/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx +++ b/frontend/src/v2/__tests__/V2DiscordCallback.test.tsx @@ -174,6 +174,19 @@ describe('V2DiscordCallback (d215 s-handoff)', () => { expect(axios.get.mock.calls[0][1]).toEqual({ headers: { Authorization: 'Bearer t' } }); }); + test('a failed channel fetch logs its status, not the install proof', async () => { + const proof = ['eyJhbGciOiJIUzI1NiJ9', 'eyJwdXIiOiJkaXNjb3JkLWluc3RhbGwifQ', 'c2lnbmF0dXJlLV8'].join('.'); + axios.get.mockRejectedValueOnce(Object.assign(new Error('Request failed'), { + config: { params: { install: proof } }, + response: { status: 404 }, + })); + const errors = jest.spyOn(console, 'error').mockImplementation(() => {}); + renderAt(`/discord/success?pod_id=p1&guild_id=123456789012345678&server_name=S&install=${encodeURIComponent(proof)}`); + expect(await screen.findByRole('alert')).toHaveTextContent('Failed to fetch server channels'); + expect(errors).toHaveBeenCalledWith('Error fetching channels:', 404); + expect(JSON.stringify(errors.mock.calls, (_k, v) => (v instanceof Error ? { ...v, message: v.message } : v))).not.toContain(proof); + }); + test('reads in Chinese under zh-CN', async () => { await act(async () => { await i18n.changeLanguage('zh-CN'); }); renderAt('/discord/error'); diff --git a/frontend/src/v2/components/V2DiscordCallback.tsx b/frontend/src/v2/components/V2DiscordCallback.tsx index 448f19d13..4836034c2 100644 --- a/frontend/src/v2/components/V2DiscordCallback.tsx +++ b/frontend/src/v2/components/V2DiscordCallback.tsx @@ -71,7 +71,9 @@ const V2DiscordCallback: React.FC = ({ type = 'callback' }); setChannels(response.data); } catch (err) { - console.error('Error fetching channels:', err); + // The status only: an axios error carries its request config, and with it + // the install proof (F89). + console.error('Error fetching channels:', (err as { response?: { status?: number } })?.response?.status ?? 'no response'); setError(t('auth.discordHandoff.channelsFailed')); } finally { setLoading(false); From ccf724b83010480fbf789aae593f353a5ebd864f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:16:44 -0800 Subject: [PATCH 3/3] fix(sentry): scrub the Discord install proof from breadcrumb and request URLs CodeRabbit on #3367: the scrubber dropped only 'invite', so the channel request's XHR breadcrumb kept the 30-minute proof. Reverse-checked: without the delete the new test fails. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- frontend/src/sentry.test.ts | 19 +++++++++++++++++++ frontend/src/sentry.ts | 2 ++ 2 files changed, 21 insertions(+) diff --git a/frontend/src/sentry.test.ts b/frontend/src/sentry.test.ts index f2f337e3e..d552eb408 100644 --- a/frontend/src/sentry.test.ts +++ b/frontend/src/sentry.test.ts @@ -118,6 +118,25 @@ describe('Sentry frontend instrumentation', () => { expect(ordinaryFragmentEvent.request.url).toBe('https://commonly.me/v2/register#section'); }); + // F89: the Discord success page carries a 30-minute install proof in `install` + // and sends it on the channel request, so it must not reach Sentry either. + it('scrubs the Discord install proof from breadcrumbs and request URLs', () => { + process.env.REACT_APP_SENTRY_DSN = 'https://public@example.ingest.sentry.io/456'; + loadSentryModule(); + const options = mockSentryInit.mock.calls[0][0]; + + const breadcrumb = options.beforeBreadcrumb({ + category: 'xhr', + data: { url: '/api/discord/channels/123456789012345678?install=PROOF-SECRET' }, + }); + expect(breadcrumb.data.url).toBe('/api/discord/channels/123456789012345678'); + + const event = options.beforeSend({ + request: { url: 'https://commonly.me/discord/success?pod_id=p1&guild_id=123456789012345678&install=PROOF-SECRET' }, + }); + expect(event.request.url).toBe('https://commonly.me/discord/success?pod_id=p1&guild_id=123456789012345678'); + }); + it('loads the SDK only through the DSN-gated dynamic import', () => { const indexSource = fs.readFileSync(path.join(__dirname, 'index.tsx'), 'utf8'); const viteSource = fs.readFileSync(path.join(__dirname, '../vite.config.ts'), 'utf8'); diff --git a/frontend/src/sentry.ts b/frontend/src/sentry.ts index 9d66be5e4..9969f764c 100644 --- a/frontend/src/sentry.ts +++ b/frontend/src/sentry.ts @@ -8,6 +8,8 @@ const scrubInviteFromUrl = (raw: string): string => { const isProtocolRelative = raw.startsWith('//'); const url = new URL(raw, 'https://commonly.invalid'); url.searchParams.delete('invite'); + // The Discord first-install proof (F89): a signed 30-minute credential. + url.searchParams.delete('install'); if (url.hash) { const hashParams = new URLSearchParams(url.hash.slice(1)); if (hashParams.has('invite')) {