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,