From 12c3cfe8697845589af979a5a14e83ec7a2a1e5c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 03:07:16 -0800 Subject: [PATCH] feat(connectors): a refused connect links the provider page when its grant may still be live TASK-344. When a provider grants scopes Commonly didn't ask for (or the exchange reports a scope it can't read), the callback refuses the connect and tries to withdraw the grant at the vendor. When it can't (no RFC 7009 endpoint, no token, or the vendor revoke fails), the person used to see only "the connection wasn't saved" while the vendor still listed Commonly as connected. - Backend: the refused redirect carries `entryId` + `revokeAt` (the entry's revoke page) exactly when the grant was left live; a withdrawn grant carries neither. The catalogue now projects each hosted entry's `revokePage`. - Frontend: the callback error effect captures and strips both params; the page links `revokeAt` only when it equals the catalogue entry's `revokePage` and passes safeProviderRevokePage. The link opens in a new tab with noopener noreferrer, inside the same alert as the notice. - Copy follows the per-entry revoke copy: Linear lists an MCP grant, Sentry's revoke ends every client of its MCP app, and Supabase's disconnects the whole organization, so those three say so. Reverse checks: dropping the catalogue equality, the per-entry context, or the query cleanup each turns a frontend test red; the backend redirect tests are red without `manualRevoke`, the catalogue test without the field. Frontend 2269/2269, backend 49/49 + 9/9, tsc clean on both. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../routes/hostedMcpConnect.callback.test.js | 73 +++++++++++++++++++ .../installableCatalogService.test.js | 4 + backend/routes/hostedMcpConnect.ts | 24 +++--- .../installable/installableCatalogService.ts | 1 + frontend/src/i18n/locales/en.json | 4 + frontend/src/i18n/locales/zh-CN.json | 4 + .../ConnectorsPage.channels.test.tsx | 58 +++++++++++++++ frontend/src/v2/connectors/ConnectorsPage.tsx | 31 +++++++- frontend/src/v2/connectors/channelModel.ts | 4 + .../src/v2/connectors/useChannelConnectors.ts | 25 +++++++ 10 files changed, 217 insertions(+), 11 deletions(-) diff --git a/backend/__tests__/unit/routes/hostedMcpConnect.callback.test.js b/backend/__tests__/unit/routes/hostedMcpConnect.callback.test.js index 181dfd92cd..b30c6a91e1 100644 --- a/backend/__tests__/unit/routes/hostedMcpConnect.callback.test.js +++ b/backend/__tests__/unit/routes/hostedMcpConnect.callback.test.js @@ -450,6 +450,79 @@ describe('hosted-mcp connect: callback', () => { } }); + // TASK-344: a refused grant is never stored, so when the vendor revoke can't + // be confirmed the grant is still live at the vendor and only the person can + // remove it there. The redirect names the entry's own page; it says nothing + // when the revoke went through (the arm above pins that exact shape). + it('a refused overbroad grant whose vendor revoke fails names the vendor revoke page', async () => { + FIXTURE_ENTRY.revoke.endpoint = 'https://mcp.linear.app/revoke'; + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + global.fetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ access_token: 'access-overbroad', refresh_token: 'refresh-overbroad', scope: 'read openid write' }), + }) + .mockResolvedValueOnce({ ok: false, status: 503, json: async () => ({}) }); + + try { + const res = await callback(); + + expect(outcome(res)).toEqual({ + status: 302, + hostedMcp: 'error', + code: 'unrequested_scope', + entryId: 'linear', + revokeAt: FIXTURE_ENTRY.revoke.page, + }); + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + it('a refused overbroad grant on an entry with no revoke endpoint names the vendor revoke page', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + global.fetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ access_token: 'access-overbroad', refresh_token: 'refresh-overbroad', scope: 'read openid write' }), + }); + + try { + const res = await callback(); + + expect(outcome(res)).toEqual({ + status: 302, + hostedMcp: 'error', + code: 'unrequested_scope', + entryId: 'linear', + revokeAt: FIXTURE_ENTRY.revoke.page, + }); + expect(global.fetch).toHaveBeenCalledTimes(1); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + it('an unreadable scope whose grant cannot be revoked names the vendor revoke page', async () => { + global.fetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ access_token: 'access-1', refresh_token: 'refresh-1', scope: ['read'] }), + }); + + const res = await callback(); + + expect(outcome(res)).toEqual({ + status: 302, + hostedMcp: 'error', + code: 'exchange_invalid_scope', + entryId: 'linear', + revokeAt: FIXTURE_ENTRY.revoke.page, + }); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + }); + it('accepts the exact requested scope set regardless of order or comma separators', async () => { global.fetch.mockResolvedValue({ ok: true, diff --git a/backend/__tests__/unit/services/installableCatalogService.test.js b/backend/__tests__/unit/services/installableCatalogService.test.js index 58fcda7f0f..a850a004f2 100644 --- a/backend/__tests__/unit/services/installableCatalogService.test.js +++ b/backend/__tests__/unit/services/installableCatalogService.test.js @@ -288,6 +288,10 @@ it("a hosted entry's tool row carries its connect descriptor and only its own co expect(linear.connectNotice).toBeUndefined(); expect(acmeRow.accessPage).toBe(acme.accessPage); expect(linear.accessPage).toBeUndefined(); + // The page a refused connect's error return may name (TASK-344): the + // browser shows a link only when the callback's page equals this one. + expect(acmeRow.revokePage).toBe(acme.revoke.page); + expect(linear.revokePage).toBe(linearEntry.revoke.page); // Each row sees its own vendor's connection and not the other's. expect(linear.connections).toEqual([ { connectionId: 'conn-linear', owner: '', repo: '' }, diff --git a/backend/routes/hostedMcpConnect.ts b/backend/routes/hostedMcpConnect.ts index 21a1958048..ef20208efb 100644 --- a/backend/routes/hostedMcpConnect.ts +++ b/backend/routes/hostedMcpConnect.ts @@ -95,15 +95,22 @@ const parseScopeSet = (scope: string): Set => new Set( scope.split(/[\s,]+/).map((value) => value.trim()).filter(Boolean), ); -/** A refused grant is never stored; revoke its refresh token when the entry can. */ +/** + * A refused grant is never stored; revoke its refresh token when the entry can. + * Whatever happens the connect stays refused and no token is kept. When the + * vendor did not confirm the revoke, the grant is still live there and only the + * person can remove it, so the redirect names the entry's own revoke page + * (TASK-344). The page is the server's; the browser never supplies it. + */ const revokeRefusedGrant = async ( entry: HostedMcpEntry, client: { clientId: string; clientSecret?: string }, tokens: TokenResponse, -): Promise => { - if (!hostedMcpRevokeTarget(entry)?.endpoint) return; +): Promise<{ manualRevoke?: { entryId: string; revokeAt: string } }> => { + const target = hostedMcpRevokeTarget(entry); + const leftLive = target?.page ? { manualRevoke: { entryId: entry.id, revokeAt: target.page } } : {}; const token = tokens.refresh_token || tokens.access_token; - if (!token) return; + if (!target?.endpoint || !token) return leftLive; try { await revokeTokenAtVendor({ entry, @@ -112,8 +119,9 @@ const revokeRefusedGrant = async ( token, tokenTypeHint: tokens.refresh_token ? 'refresh_token' : 'access_token', }); + return {}; } catch { - // The connect remains refused and no token is retained if vendor revoke fails. + return leftLive; } }; @@ -501,8 +509,7 @@ const finishCallback = async (req: Request, res: Response, flow: CallbackFlow) = const scopeWasReturned = Object.prototype.hasOwnProperty.call(tokens, 'scope'); if (scopeWasReturned && typeof tokens.scope !== 'string') { - await revokeRefusedGrant(entry, client, tokens); - return callbackRedirect(res, 'error', 'exchange_invalid_scope'); + return callbackRedirect(res, 'error', 'exchange_invalid_scope', await revokeRefusedGrant(entry, client, tokens)); } const grantedScope = scopeWasReturned ? tokens.scope as string : entry.scopes.join(' '); const grantedScopes: Set = scopeWasReturned @@ -517,8 +524,7 @@ const finishCallback = async (req: Request, res: Response, flow: CallbackFlow) = entryId: entry.id, extraScopeCount: extraScopes.length, })); - await revokeRefusedGrant(entry, client, tokens); - return callbackRedirect(res, 'error', 'unrequested_scope'); + return callbackRedirect(res, 'error', 'unrequested_scope', await revokeRefusedGrant(entry, client, tokens)); } // Store the parsed set in the canonical OAuth space-delimited form. This // keeps a vendor's comma-separated response useful to later scope checks. diff --git a/backend/services/installable/installableCatalogService.ts b/backend/services/installable/installableCatalogService.ts index 43a6541c94..f12460617a 100644 --- a/backend/services/installable/installableCatalogService.ts +++ b/backend/services/installable/installableCatalogService.ts @@ -173,6 +173,7 @@ const toolEntriesFor = async (userId: string): Promise<{ entries: unknown[]; hid description: row.description || '', ...(hostedEntry?.connectNotice ? { connectNotice: hostedEntry.connectNotice } : {}), ...(hostedEntry?.accessPage ? { accessPage: hostedEntry.accessPage } : {}), + ...(hostedEntry?.revoke?.page ? { revokePage: hostedEntry.revoke.page } : {}), connectionType: meta.connectionType, ...(meta.entryId ? { entryId: meta.entryId } : {}), available: readiness.available, diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index b12793f2ac..61ebf99c4d 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2424,6 +2424,10 @@ "hostedMcpManualRevoke_supabase": "Commonly may still be authorized in the Supabase organization you connected before. Removing Commonly under Authorized apps disconnects everyone who connected that organization.", "hostedMcpReviewAuthorization": "Open {{provider}}'s connected apps", "hostedMcpReviewAuthorization_supabase": "Open Supabase Authorized apps", + "hostedMcpRefusedStillListed": "{{provider}} may still list Commonly as connected. Remove it there.", + "hostedMcpRefusedStillListed_linear": "Linear may still list this as an MCP grant under its connected apps. Revoke it there.", + "hostedMcpRefusedStillListed_sentry": "Sentry may still list its MCP server app as connected. Revoking it there ends every client that server holds for the account, not only Commonly.", + "hostedMcpRefusedStillListed_supabase": "Commonly may still be authorized in the Supabase organization you chose. Removing it under Authorized apps disconnects everyone who connected that organization.", "hostedMcpAuthenticationRequired": "Sign in to Commonly, then try connecting again.", "hostedMcpClientSetup": "This connector’s sign-in setup needs attention. Ask an administrator to check it.", "hostedMcpConsentDenied": "You didn’t approve this connection. Nothing was saved.", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index 58be3e9018..0edb2a52a2 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -2414,6 +2414,10 @@ "hostedMcpManualRevoke_supabase": "Commonly 可能仍在你之前连接的 Supabase 组织中获得授权。在“已授权应用”中移除 Commonly，会断开所有连接该组织的成员。", "hostedMcpReviewAuthorization": "打开 {{provider}} 的已连接应用", "hostedMcpReviewAuthorization_supabase": "打开 Supabase 已授权应用", + "hostedMcpRefusedStillListed": "{{provider}} 可能仍显示 Commonly 已连接，请到那里移除。", + "hostedMcpRefusedStillListed_linear": "Linear 的已连接应用中可能仍有这项 MCP 授权，请到那里撤销。", + "hostedMcpRefusedStillListed_sentry": "Sentry 中可能仍显示它的 MCP 服务器应用已连接。在那里撤销，会断开此账号下该服务器的所有客户端，不只是 Commonly。", + "hostedMcpRefusedStillListed_supabase": "Commonly 可能仍在你刚才选择的 Supabase 组织中获得授权。在“已授权应用”中移除，会断开所有连接该组织的成员。", "hostedMcpAuthenticationRequired": "请先登录 Commonly，再重新连接。", "hostedMcpClientSetup": "此连接器的登录配置有问题，请联系管理员检查。", "hostedMcpConsentDenied": "你没有同意此次授权。连接未保存。", diff --git a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx index 2f71ccd3a3..71ea3224e5 100644 --- a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx +++ b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx @@ -595,6 +595,64 @@ describe('Connectors page: channels', () => { expect(screen.queryByText('unrequested_scope')).toBeNull(); }); + // TASK-344: a refused grant is never stored, so when the vendor revoke can't be + // confirmed the grant is still live there. The link comes from the catalogue's + // own page for that entry; the callback's value only has to equal it. + const refusedReturn = (revokeAt) => ( + `/v2/connectors?hostedMcp=error&code=unrequested_scope&entryId=linear&revokeAt=${encodeURIComponent(revokeAt)}` + ); + + it('a refused connect whose grant may still be live links the provider revoke page', async () => { + const revokePage = 'https://linear.app/settings/security'; + window.history.replaceState({}, '', refusedReturn(revokePage)); + mockGets([], [{ ...hostedLinearCatalogEntry(), connections: [], revokePage }]); + renderPage(); + + const alert = await screen.findByRole('alert'); + expect(alert).toHaveTextContent("The provider granted access Commonly didn’t ask for, so the connection wasn’t saved."); + const link = await within(alert).findByRole('link', { name: "Open Linear's connected apps" }); + expect(link).toHaveAttribute('href', revokePage); + expect(link).toHaveAttribute('target', '_blank'); + expect(link).toHaveAttribute('rel', 'noopener noreferrer'); + expect(alert).toHaveTextContent('Linear may still list this as an MCP grant under its connected apps. Revoke it there.'); + expect(window.location.search).toBe(''); + }); + + // Removing Commonly in Supabase ends it for the whole organization, so a + // refused connect there must say so rather than just say "remove it". + it('a refused Supabase connect warns that removing it there disconnects the organization', async () => { + const revokePage = 'https://supabase.com/dashboard/org/_/apps'; + window.history.replaceState({}, '', `/v2/connectors?hostedMcp=error&code=unrequested_scope&entryId=supabase&revokeAt=${encodeURIComponent(revokePage)}`); + mockGets([], [{ + ...hostedLinearCatalogEntry(), installableId: 'supabase', label: 'Supabase', entryId: 'supabase', connections: [], revokePage, + }]); + renderPage(); + + const alert = await screen.findByRole('alert'); + const link = await within(alert).findByRole('link', { name: 'Open Supabase Authorized apps' }); + expect(link).toHaveAttribute('href', revokePage); + expect(alert).toHaveTextContent('Removing it under Authorized apps disconnects everyone who connected that organization.'); + }); + + // Equality with the catalogue's page, not a same-origin check: the right + // host on another path is as forged as another host. + it.each([ + ['another host', 'https://evil.example/'], + ['the provider host on another path', 'https://linear.app/evil-path'], + ['a script URL', 'javascript:alert(1)'], + ])('a refused connect never links a revoke page the catalogue does not name (%s)', async (_label, forged) => { + window.history.replaceState({}, '', refusedReturn(forged)); + mockGets([], [{ ...hostedLinearCatalogEntry(), connections: [], revokePage: 'https://linear.app/settings/security' }]); + renderPage(); + + const alert = await screen.findByRole('alert'); + expect(alert).toHaveTextContent("The provider granted access Commonly didn’t ask for, so the connection wasn’t saved."); + await waitFor(() => expect(window.location.search).toBe('')); + await act(async () => { await new Promise((resolve) => { setTimeout(resolve, 0); }); }); + expect(within(alert).queryByRole('link')).toBeNull(); + expect(document.body.innerHTML).not.toContain(forged); + }); + it.each([ ['unknown_entry', 'hostedMcpUnknownEntry'], ['invalid_state', 'hostedMcpInvalidState'], diff --git a/frontend/src/v2/connectors/ConnectorsPage.tsx b/frontend/src/v2/connectors/ConnectorsPage.tsx index 445775012e..b00075a32e 100644 --- a/frontend/src/v2/connectors/ConnectorsPage.tsx +++ b/frontend/src/v2/connectors/ConnectorsPage.tsx @@ -9,7 +9,7 @@ import { useNavigate } from 'react-router-dom'; import { catalogDescription } from '../utils/connectorCatalogDescription'; import { ADD_PLATFORMS, TYPE_LABELS } from './channelModel'; import type { ListItem } from './channelModel'; -import { entryCovers, entryIsGranted, isDead } from './toolModel'; +import { PER_ENTRY_REVOKE_COPY, entryCovers, entryIsGranted, isDead } from './toolModel'; import type { ToolCatalogEntry, ToolGrant } from './toolModel'; import { useChannelConnectors } from './useChannelConnectors'; import { useToolConnectors } from './useToolConnectors'; @@ -505,9 +505,36 @@ const ConnectorsPage: React.FC = () => { const errorText = c.hostedMcpCallbackError || c.error || c.slackCallbackError || ( !m.draft && !m.selected && !m.githubAppSetup ? m.error : null ); + // A refused connect that may have left a grant live at the provider. The + // callback error leads errorText, so this always sits beside its own notice. + const callbackRevoke = c.hostedMcpCallbackError ? c.hostedMcpCallbackRevoke : null; + // Per-entry copy names what that vendor's page lists, and what removing it there ends. + const callbackRevokeContext = callbackRevoke && PER_ENTRY_REVOKE_COPY.has(callbackRevoke.entryId) + ? callbackRevoke.entryId : undefined; const noticeBlock = ( <> - {errorText &&

{errorText}

} + {errorText && ( +

+ {errorText} + {callbackRevoke && ( + <> + {' '} + {t('connectors.hostedMcpRefusedStillListed', { + defaultValue: '{{provider}} may still list Commonly as connected. Remove it there.', + context: callbackRevokeContext, + provider: callbackRevoke.provider, + })}{' '} + + {t('connectors.hostedMcpReviewAuthorization', { + defaultValue: "Open {{provider}}'s connected apps", + context: callbackRevokeContext, + provider: callbackRevoke.provider, + })} + + + )} +

+ )} {m.removedHiddenNotice && (

{m.removedHiddenNotice.revokePage diff --git a/frontend/src/v2/connectors/channelModel.ts b/frontend/src/v2/connectors/channelModel.ts index 66638aa54e..8410922bc8 100644 --- a/frontend/src/v2/connectors/channelModel.ts +++ b/frontend/src/v2/connectors/channelModel.ts @@ -89,6 +89,10 @@ export interface CatalogEntry { unavailableReason?: string; // Tool entries provide the provider name for the hosted connection row. entryId?: string; + // The entry's own revoke page. A refused connect can leave a grant live at + // the provider, and the callback's `revokeAt` is linked only when it equals + // this server-owned value. + revokePage?: string; installation: CatalogInstallation | null; integration: Connector | null; } diff --git a/frontend/src/v2/connectors/useChannelConnectors.ts b/frontend/src/v2/connectors/useChannelConnectors.ts index 4139b93f4e..133a2dd2a8 100644 --- a/frontend/src/v2/connectors/useChannelConnectors.ts +++ b/frontend/src/v2/connectors/useChannelConnectors.ts @@ -67,6 +67,13 @@ export const useChannelConnectors = () => { const [slackCallbackError, setSlackCallbackError] = useState(null); const [slackLeadChoices, setSlackLeadChoices] = useState>({}); const [hostedMcpCallbackError, setHostedMcpCallbackError] = useState(null); + // A refused connect whose grant the server could not withdraw. `pending` is + // what the callback URL claimed; `hostedMcpCallbackRevoke` is set only once + // the catalogue confirms it. + const [pendingCallbackRevoke, setPendingCallbackRevoke] = useState<{ entryId: string; revokeAt: string } | null>(null); + const [hostedMcpCallbackRevoke, setHostedMcpCallbackRevoke] = useState<{ + entryId: string; provider: string; revokePage: string; + } | null>(null); const pollRef = useRef | null>(null); const podPickerRef = useRef(null); // Ages must advance while the page sits open, and the source must be re-read @@ -135,8 +142,13 @@ export const useChannelConnectors = () => { const query = new URLSearchParams(window.location.search); if (query.get('hostedMcp') !== 'error') return; setHostedMcpCallbackError(hostedMcpErrorNotice(t, query.get('code'))); + const entryId = query.get('entryId'); + const revokeAt = query.get('revokeAt'); + if (entryId && revokeAt) setPendingCallbackRevoke({ entryId, revokeAt }); query.delete('hostedMcp'); query.delete('code'); + query.delete('entryId'); + query.delete('revokeAt'); query.delete('extraScope'); query.delete('error'); query.delete('error_description'); @@ -171,6 +183,18 @@ export const useChannelConnectors = () => { ); }, [load, t]); + // A refused connect has no row to read, so the catalogue entry is the + // authority: the page is linked only when it is the one the entry names. + useEffect(() => { + if (!pendingCallbackRevoke || loading || hostedCatalog === null) return; + const entry = hostedCatalog.find((candidate) => candidate.entryId === pendingCallbackRevoke.entryId); + const { revokeAt } = pendingCallbackRevoke; + if (entry?.label && entry.revokePage === revokeAt && safeProviderRevokePage(revokeAt)) { + setHostedMcpCallbackRevoke({ entryId: pendingCallbackRevoke.entryId, provider: entry.label, revokePage: revokeAt }); + } + setPendingCallbackRevoke(null); + }, [hostedCatalog, loading, pendingCallbackRevoke]); + // The hosted-MCP callback says when a previous provider authorization may // need manual attention. Resolve its page from the authenticated connection // row and match the callback copy to that server-owned value; never turn an @@ -977,6 +1001,7 @@ export const useChannelConnectors = () => { setSlackLeadChoices, hostedMcpCallbackError, setHostedMcpCallbackError, + hostedMcpCallbackRevoke, pollRef, podPickerRef, now,