diff --git a/apps/desktop/src/api/mcp.ts b/apps/desktop/src/api/mcp.ts index f67870e5bf..94a22dc627 100644 --- a/apps/desktop/src/api/mcp.ts +++ b/apps/desktop/src/api/mcp.ts @@ -48,31 +48,17 @@ export function saveMcpServers( }) } -/** Start an MCP OAuth flow and return the authorization URL. */ -export function authMcpServer(name: string, profile?: ProfileScope): Promise { - return window.hermesDesktop.api({ - ...capabilityScoped(profile), - path: `/api/mcp/servers/${encodeURIComponent(name)}/auth`, - method: 'POST', - timeoutMs: 60_000 - }) -} +/** Capture the source before the first await. Every OAuth RPC, including + * cleanup after a foreground switch, belongs to this (connection, profile). + * Import the store lazily: it consumes the API barrel during initialization. */ +export function mcpOAuthRpc(scope?: ProfileScope) { + const { connectionId = null, profile = 'default' } = capabilityScoped(scope) -export function getMcpOAuthFlow(flowId: string, profile?: ProfileScope): Promise { - return window.hermesDesktop.api({ - ...capabilityScoped(profile), - path: `/api/mcp/oauth/flows/${encodeURIComponent(flowId)}` - }) -} + return async (action: 'start' | 'poll' | 'callback' | 'cancel', params: Record): Promise => { + const { requestGatewayForAgent } = await import('@/store/gateway') -/** Cancel an in-flight MCP OAuth flow server-side, freeing the per-server - * "already in progress" slot so a retry doesn't 409. */ -export function cancelMcpOAuthFlow(flowId: string, profile?: null | string): Promise<{ ok: boolean; status: string }> { - return hermesApi<{ ok: boolean; status: string }>({ - ...profileScoped(profile), - path: `/api/mcp/oauth/flows/${encodeURIComponent(flowId)}`, - method: 'DELETE' - }) + return requestGatewayForAgent(connectionId, profile, `mcp.servers.oauth.${action}`, params, 60_000) + } } // --------------------------------------------------------------------------- @@ -90,16 +76,19 @@ export function listMcpServers(): Promise<{ servers: McpServerSummary[] }> { /** Add one server to `mcp_servers` (validated + name-collision-checked * server-side — the same endpoint the dashboard's add form uses). */ -export function addMcpServer(body: { - name: string - url?: string - command?: string - args?: string[] - env?: Record - auth?: string -}): Promise { - return hermesApi({ - ...profileScoped(), +export function addMcpServer( + body: { + name: string + url?: string + command?: string + args?: string[] + env?: Record + auth?: string + }, + profile?: ProfileScope +): Promise { + return window.hermesDesktop.api({ + ...capabilityScoped(profile), path: '/api/mcp/servers', method: 'POST', body @@ -108,9 +97,9 @@ export function addMcpServer(body: { /** Remove one server from `mcp_servers` (the inline setup card's rollback * when a directory install is cancelled after the config write). */ -export function removeMcpServer(name: string): Promise<{ ok: boolean }> { - return hermesApi<{ ok: boolean }>({ - ...profileScoped(), +export function removeMcpServer(name: string, profile?: ProfileScope): Promise<{ ok: boolean }> { + return window.hermesDesktop.api<{ ok: boolean }>({ + ...capabilityScoped(profile), path: `/api/mcp/servers/${encodeURIComponent(name)}`, method: 'DELETE' }) diff --git a/apps/desktop/src/app/skills/mcp-tab.tsx b/apps/desktop/src/app/skills/mcp-tab.tsx index 7ef859479c..b31bd5a766 100644 --- a/apps/desktop/src/app/skills/mcp-tab.tsx +++ b/apps/desktop/src/app/skills/mcp-tab.tsx @@ -17,11 +17,9 @@ import { TextTab } from '@/components/ui/text-tab' import { Textarea } from '@/components/ui/textarea' import { Tip } from '@/components/ui/tooltip' import { - authMcpServer, getActionStatus, getLogs, getMcpCatalog, - getMcpOAuthFlow, getUsageAnalytics, type HermesGateway, installMcpCatalogEntry, @@ -611,9 +609,8 @@ export function McpTab({ gateway, profile }: { gateway: HermesGateway | null; pr try { const flow = await completeMcpDesktopOAuth({ serverName, - start: name => authMcpServer(name, profile ?? undefined), - status: flowId => getMcpOAuthFlow(flowId, profile ?? undefined), - openExternal: url => window.hermesDesktop.openExternal(url) + profile, + cancelled: () => profileEpoch.current !== epoch }) const result: McpTestResult = { ok: true, tools: flow.tools ?? [] } diff --git a/apps/desktop/src/components/assistant-ui/mcp-setup-tool.tsx b/apps/desktop/src/components/assistant-ui/mcp-setup-tool.tsx index 2b7e4e85fa..99d4ee23e5 100644 --- a/apps/desktop/src/components/assistant-ui/mcp-setup-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/mcp-setup-tool.tsx @@ -1,4 +1,5 @@ -'use client' +import { capabilityScoped } from '@/api/client' +;('use client') import { type ToolCallMessagePartProps, useAuiState } from '@assistant-ui/react' import { useStore } from '@nanostores/react' @@ -12,11 +13,8 @@ import { Codicon } from '@/components/ui/codicon' import { Input } from '@/components/ui/input' import { addMcpServer, - authMcpServer, - cancelMcpOAuthFlow, getActionStatus, getMcpCatalog, - getMcpOAuthFlow, installMcpCatalogEntry, type McpCatalogEntry, removeMcpServer, @@ -250,6 +248,7 @@ function McpSetupPending({ args }: ToolCallMessagePartProps) { const approve = useCallback(async () => { cancelRef.current = false + const oauthScope = capabilityScoped() setWorking(true) // Poll-boundary abort for the background-install loop; the OAuth flows @@ -274,11 +273,8 @@ function McpSetupPending({ args }: ToolCallMessagePartProps) { if (action === 'authorize') { const flow = await completeMcpDesktopOAuth({ serverName: server, - start: authMcpServer, - status: getMcpOAuthFlow, - cancelled: () => cancelRef.current, - cancel: cancelMcpOAuthFlow, - openExternal: url => window.hermesDesktop.openExternal(url) + profile: oauthScope, + cancelled: () => cancelRef.current }) triggerHaptic('submit') @@ -314,21 +310,18 @@ function McpSetupPending({ args }: ToolCallMessagePartProps) { // flow dies after the config write (cancel, closed OAuth tab), roll // the write back — decline means "no server", not an unauthorized // entry squatting in mcp_servers (authoritative-write rule). - await addMcpServer({ name: known.name, url: known.url }) + await addMcpServer({ name: known.name, url: known.url }, oauthScope) let flow try { flow = await completeMcpDesktopOAuth({ serverName: known.name, - start: authMcpServer, - status: getMcpOAuthFlow, - cancelled: () => cancelRef.current, - cancel: cancelMcpOAuthFlow, - openExternal: url => window.hermesDesktop.openExternal(url) + profile: oauthScope, + cancelled: () => cancelRef.current }) } catch (error) { - await removeMcpServer(known.name).catch(() => { + await removeMcpServer(known.name, oauthScope).catch(() => { // Rollback is best-effort; the primary error/cancel wins. }) throw error diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts index 0c2e315134..292c656dd2 100644 --- a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts @@ -1,72 +1,278 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' -import { completeMcpDesktopOAuth } from './mcp-dashboard-oauth' +import { setApiRequestConnection, setApiRequestProfile } from '@/api/client' +import { requestGatewayForAgent } from '@/store/gateway' -describe('completeMcpDesktopOAuth', () => { - it('opens the returned authorization URL and polls through approval', async () => { - const openExternal = vi.fn().mockResolvedValue(undefined) +import { completeMcpDesktopOAuth, McpOAuthCancelled } from './mcp-dashboard-oauth' - const status = vi - .fn() - .mockResolvedValueOnce({ - flow_id: 'flow-1', - server_name: 'reports', - status: 'authorization_required', - authorization_url: 'https://idp.example/authorize', - error: null - }) - .mockResolvedValueOnce({ - flow_id: 'flow-1', - server_name: 'reports', - status: 'approved', - authorization_url: 'https://idp.example/authorize', - error: null, - tools: [{ name: 'list_reports', description: 'List reports' }] - }) +vi.mock('@/store/gateway', () => ({ requestGatewayForAgent: vi.fn() })) - const result = await completeMcpDesktopOAuth({ - serverName: 'reports', - start: vi.fn().mockResolvedValue({ - flow_id: 'flow-1', - server_name: 'reports', - status: 'authorization_required', - authorization_url: 'https://idp.example/authorize', - error: null - }), - status, - openExternal, - sleep: async () => {} - }) +const redirectUri = 'http://127.0.0.1:49152/callback' +const authUrl = `https://idp.example/authorize?state=expected&redirect_uri=${encodeURIComponent(redirectUri)}` +const started = { ok: true, session_id: 'flow-1', auth_url: authUrl, flow: 'pkce' } +const tools = [{ name: 'list_reports', description: 'List reports' }] - expect(openExternal).toHaveBeenCalledWith('https://idp.example/authorize') - expect(result.status).toBe('approved') +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: Error) => void + + const promise = new Promise((res, rej) => { + resolve = res + reject = rej }) - it('retries a transient status failure', async () => { - const status = vi.fn().mockRejectedValueOnce(new Error('temporary network failure')).mockResolvedValueOnce({ - flow_id: 'flow-2', - server_name: 'reports', - status: 'approved', - authorization_url: 'https://idp.example/authorize', - error: null, - tools: [] + return { promise, resolve, reject } +} + +function harness() { + const callback = deferred<{ code: string | null; state: string | null; error: string | null }>() + + const bridge = { + listen: vi.fn().mockResolvedValue({ id: 'listener-1', redirectUri }), + wait: vi.fn(() => callback.promise), + cancel: vi.fn(async () => { + callback.resolve({ code: null, state: null, error: 'cancelled' }) + + return true + }) + } + + const api = vi.fn().mockRejectedValue(new Error('Desktop OAuth must not use the remote REST callback')) + + const openExternal = vi.fn(async () => { + callback.resolve({ code: 'code-1', state: 'expected', error: null }) + }) + + Object.defineProperty(window, 'hermesDesktop', { configurable: true, value: { mcpOauth: bridge, api, openExternal } }) + let relayed = false + const rpc = vi.mocked(requestGatewayForAgent) + rpc.mockImplementation(async (_connection, _profile, method) => { + if (method.endsWith('.start')) { + return started + } + + if (method.endsWith('.callback')) { + relayed = true + + return { ok: true } + } + + if (method.endsWith('.cancel')) { + return { ok: true, status: 'error' } + } + + return { ok: true, status: relayed ? 'approved' : 'pending', tools } + }) + + return { bridge, callback, api, openExternal, rpc } +} + +afterEach(() => { + vi.resetAllMocks() + setApiRequestConnection(null) + setApiRequestProfile(null) +}) + +describe('Desktop MCP client callback lifecycle', () => { + it('bounds pending polls even after the browser callback arrives', async () => { + const { rpc, bridge } = harness() + const base = rpc.getMockImplementation()! + rpc.mockImplementation(async (...args) => + args[2].endsWith('.poll') ? { ok: true, status: 'pending' } : base(...args) + ) + let now = 0 + const clock = vi.spyOn(Date, 'now').mockImplementation(() => now) + + try { + await expect( + completeMcpDesktopOAuth({ + serverName: 'reports', + timeoutMs: 3000, + sleep: async () => { + now += 1000 + } + }) + ).rejects.toThrow('Timed out') + expect(bridge.cancel).toHaveBeenCalled() + } finally { + clock.mockRestore() + } + }) + + it.each([null, 'local', 'remote-gateway'])( + 'relays the client callback and pins %s plus profile across foreground switches', + async connectionId => { + const { bridge, api, openExternal, rpc } = harness() + setApiRequestConnection(connectionId) + setApiRequestProfile('origin-profile') + openExternal.mockImplementation(async () => { + setApiRequestConnection('other-gateway') + setApiRequestProfile('other-profile') + const cb = { code: 'code-1', state: 'expected', error: null } + bridge.wait.mockResolvedValue(cb) + + // The waiter was already armed before the browser opened. + return undefined + }) + const callbackResult = { code: 'code-1', state: 'expected', error: null } + bridge.wait.mockResolvedValue(callbackResult) + + const result = await completeMcpDesktopOAuth({ serverName: 'reports', sleep: async () => {} }) + + expect(result).toMatchObject({ status: 'approved', tools }) + expect(openExternal).toHaveBeenCalledWith(authUrl) + expect(rpc).toHaveBeenCalledWith( + connectionId, + 'origin-profile', + 'mcp.servers.oauth.start', + { + name: 'reports', + client_redirect_uri: redirectUri + }, + 60_000 + ) + expect(rpc).toHaveBeenCalledWith( + connectionId, + 'origin-profile', + 'mcp.servers.oauth.callback', + { + name: 'reports', + session_id: 'flow-1', + ...callbackResult + }, + 60_000 + ) + expect(rpc.mock.calls.every(call => call[0] === connectionId && call[1] === 'origin-profile')).toBe(true) + expect(bridge.cancel).toHaveBeenCalledWith('listener-1') + expect(api).not.toHaveBeenCalled() + } + ) + + it.each([ + 'cancel-start', + 'cancel-poll', + 'browser', + 'listen', + 'start', + 'poll', + 'relay', + 'wait', + 'legacy', + 'missing-method', + 'denial', + 'missing-bridge' + ])('cleans up on %s without retargeting or retrying a remote callback', async failure => { + const { bridge, callback, api, openExternal, rpc } = harness() + setApiRequestConnection('remote-gateway') + setApiRequestProfile('origin-profile') + let cancelled = false + const pendingStart = deferred() + const base = rpc.getMockImplementation()! + rpc.mockImplementation(async (connection, profile, method, params, timeout) => { + if (method.endsWith('.start')) { + if (failure === 'missing-method') { + throw new Error('Unknown method: mcp.servers.oauth.start') + } + + if (failure === 'start') { + throw new Error('start failed') + } + + if (failure === 'cancel-start') { + cancelled = true + setApiRequestConnection('other-gateway') + setApiRequestProfile('other-profile') + pendingStart.resolve(started) + + return pendingStart.promise + } + + if (failure === 'legacy') { + return { + ...started, + auth_url: 'https://idp.example/authorize?state=expected&redirect_uri=http%3A%2F%2Fremote%2Fcallback' + } + } + } + + if (method.endsWith('.poll')) { + if (failure === 'denial') { + return { ok: true, status: 'error', error_message: 'access_denied' } + } + + if (failure === 'poll') { + throw new Error('poll failed') + } + + if (failure === 'cancel-poll') { + cancelled = true + + return { ok: true, status: 'pending' } + } + } + + if (method.endsWith('.callback') && failure === 'relay') { + return { ok: false, error_message: 'state mismatch' } + } + + return base(connection, profile, method, params, timeout) }) - const result = await completeMcpDesktopOAuth({ - serverName: 'reports', - start: vi.fn().mockResolvedValue({ - flow_id: 'flow-2', - server_name: 'reports', - status: 'authorization_required', - authorization_url: 'https://idp.example/authorize', - error: null - }), - status, - openExternal: vi.fn().mockResolvedValue(undefined), - sleep: async () => {} - }) + if (failure === 'browser') { + openExternal.mockRejectedValue(new Error('browser failed')) + } - expect(result.status).toBe('approved') - expect(status).toHaveBeenCalledTimes(2) + if (failure === 'listen') { + bridge.listen.mockRejectedValue(new Error('listen failed')) + } + + if (failure === 'wait') { + bridge.wait.mockRejectedValue(new Error('wait failed')) + } + + if (failure === 'missing-bridge') { + window.hermesDesktop.mcpOauth = undefined + } + + if (failure === 'cancel-poll' || failure === 'poll') { + openExternal.mockResolvedValue(undefined) + } + + const action = completeMcpDesktopOAuth({ serverName: 'reports', cancelled: () => cancelled, sleep: async () => {} }) + + if (failure.startsWith('cancel-')) { + await expect(action).rejects.toBeInstanceOf(McpOAuthCancelled) + } else { + await expect(action).rejects.toThrow() + } + + if (!['listen', 'missing-bridge'].includes(failure)) { + expect(bridge.cancel).toHaveBeenCalledWith('listener-1') + } + + if (!['listen', 'start', 'missing-method', 'missing-bridge'].includes(failure)) { + expect(rpc).toHaveBeenCalledWith( + 'remote-gateway', + 'origin-profile', + 'mcp.servers.oauth.cancel', + { + name: 'reports', + session_id: 'flow-1' + }, + 60_000 + ) + } + + expect(rpc.mock.calls.every(call => call[0] === 'remote-gateway' && call[1] === 'origin-profile')).toBe(true) + + if (failure === 'cancel-start' || failure === 'legacy') { + expect(openExternal).not.toHaveBeenCalled() + } + + expect(rpc.mock.calls.filter(call => call[2].endsWith('.start'))).toHaveLength( + failure === 'listen' || failure === 'missing-bridge' ? 0 : 1 + ) + expect(api).not.toHaveBeenCalled() + callback.resolve({ code: null, state: null, error: 'cancelled' }) }) }) diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.ts index 32b56362de..cee1506ebf 100644 --- a/apps/desktop/src/lib/mcp-dashboard-oauth.ts +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.ts @@ -1,30 +1,27 @@ -export interface McpOAuthFlow { - flow_id: string - server_name: string - status: 'starting' | 'authorization_required' | 'approved' | 'error' - authorization_url: string | null - error: string | null - tools?: Array<{ name: string; description: string }> -} +import type { ProfileScope } from '@/api/client' +import { type McpOAuthFlow, mcpOAuthRpc } from '@/api/mcp' + +import { isMissingRpcMethod } from './gateway-rpc' interface CompleteOptions { serverName: string - start: (name: string) => Promise - status: (flowId: string) => Promise - openExternal: (url: string) => Promise - /** Polled between status checks. Returning true cancels: the flow is - * cancelled SERVER-SIDE (freeing the per-server in-progress slot — without - * it a retry 409s until the backend's callback timeout) and the promise - * rejects with `McpOAuthCancelled`. */ + profile?: ProfileScope cancelled?: () => boolean - /** Server-side flow cancel, wired to DELETE /api/mcp/oauth/flows/{id}. */ - cancel?: (flowId: string) => Promise sleep?: (milliseconds: number) => Promise maxPollFailures?: number + timeoutMs?: number } -/** Thrown when the caller's `cancelled()` tripped — callers branch on this to - * skip error toasts for a deliberate user cancel. */ +interface OAuthResult { + ok: boolean + session_id?: string + auth_url?: string + status?: 'pending' | 'approved' | 'error' + error_message?: string + tools?: McpOAuthFlow['tools'] +} + +/** Deliberate cancellation is not an error toast. */ export class McpOAuthCancelled extends Error { constructor() { super('OAuth cancelled by user') @@ -33,64 +30,172 @@ export class McpOAuthCancelled extends Error { } const defaultSleep = (milliseconds: number) => new Promise(resolve => window.setTimeout(resolve, milliseconds)) +const UPDATE_BACKEND = 'Update the Hermes backend to support Desktop MCP OAuth callbacks.' +/** The browser and redirect listener live on the Desktop machine, even when + * the token-exchanging gateway is remote. Never fall back to its REST callback. */ export async function completeMcpDesktopOAuth({ serverName, - start, - status, - openExternal, + profile, cancelled, - cancel, sleep = defaultSleep, - maxPollFailures = 3 + maxPollFailures = 3, + timeoutMs = 360_000 }: CompleteOptions): Promise { - const started = await start(serverName) + const deadline = Date.now() + timeoutMs + const rpc = mcpOAuthRpc(profile) + const bridge = window.hermesDesktop.mcpOauth - if (started.status === 'error') { - throw new Error(started.error || 'OAuth failed to start') + if (!bridge) { + throw new Error('Update Hermes Desktop to support MCP OAuth callbacks.') } - if (!started.authorization_url) { - throw new Error('OAuth server did not provide an authorization URL') - } + let listener: { id: string; redirectUri: string } | undefined + let sessionId: string | undefined + let authUrl: string | undefined + let approved = false + let closed = false + let relayError: unknown - await openExternal(started.authorization_url) - - let pollFailures = 0 - - for (;;) { + const checkCancelled = () => { if (cancelled?.()) { - // Free the backend slot before rejecting; best-effort — the flow also - // dies on its own timeout if this request is lost. - await cancel?.(started.flow_id).catch(() => {}) throw new McpOAuthCancelled() } + } - let current: McpOAuthFlow + const request = async (action: 'start' | 'poll' | 'callback' | 'cancel', params: Record) => { + const result = await rpc(action, { name: serverName, ...params }) - try { - current = await status(started.flow_id) - pollFailures = 0 - } catch (error) { - pollFailures += 1 + if (!result.ok) { + throw new Error(result.error_message || 'MCP OAuth request failed') + } - if (pollFailures >= maxPollFailures) { - throw error + return result + } + + try { + checkCancelled() + listener = await bridge.listen() + checkCancelled() + const started = await request('start', { client_redirect_uri: listener.redirectUri }) + sessionId = started.session_id + authUrl = started.auth_url + // Start may have created a flow while the user cancelled. Keep its id so + // finally can release it, but do not launch a browser after cancellation. + checkCancelled() + + if (!sessionId || !authUrl) { + throw new Error('OAuth server did not provide an authorization URL and session') + } + + // Pre-relay backends silently ignore unknown start params. Do not open an + // authorization URL whose DCR/PKCE flow points at a different machine. + if (new URL(authUrl).searchParams.get('redirect_uri') !== listener.redirectUri) { + throw new Error(UPDATE_BACKEND) + } + + const flowId = sessionId + void bridge + .wait(listener.id) + .then(async callback => { + if (closed || cancelled?.()) { + return + } + + if (!callback.state) { + throw new Error(callback.error || 'OAuth callback did not include state') + } + + await request('callback', { session_id: flowId, ...callback }) + }) + .catch(error => { + relayError = error + }) + + await window.hermesDesktop.openExternal(authUrl) + let pollFailures = 0 + + for (;;) { + checkCancelled() + + if (Date.now() >= deadline) { + throw new Error('Timed out waiting for MCP OAuth authorization') + } + + if (relayError) { + throw relayError + } + + let current: OAuthResult + + try { + current = await request('poll', { session_id: flowId }) + pollFailures = 0 + } catch (error) { + if (++pollFailures >= maxPollFailures) { + throw error + } + + await sleep(1000) + + continue + } + + checkCancelled() + + if (relayError) { + throw relayError + } + + if (current.status === 'approved') { + approved = true + + return { + flow_id: flowId, + server_name: serverName, + status: 'approved', + authorization_url: authUrl, + error: null, + tools: current.tools + } + } + + if (current.status === 'error') { + throw new Error(current.error_message || 'OAuth authorization failed') } await sleep(1000) - - continue + } + } catch (error) { + if (isMissingRpcMethod(error)) { + throw new Error(UPDATE_BACKEND) } - if (current.status === 'approved') { - return current + throw error + } finally { + closed = true + + // Stop the native waiter first: its synthetic cancellation is NOT a + // provider callback and must never race cleanup back onto the gateway. + if (listener) { + await bridge.cancel(listener.id).catch(() => {}) } - if (current.status === 'error') { - throw new Error(current.error || 'OAuth authorization failed') - } + if (sessionId && !approved) { + try { + await request('cancel', { session_id: sessionId }) + } catch (error) { + // Relay-capable backends predating oauth.cancel can still release a + // waiting worker with a state-checked denial. No HTTP redirect retry. + if (isMissingRpcMethod(error) && authUrl) { + const state = new URL(authUrl).searchParams.get('state') - await sleep(1000) + if (state) { + await request('callback', { session_id: sessionId, state, error: 'access_denied' }).catch(() => {}) + } + } + // Network cleanup is best-effort; backend callback timeout is bounded. + } + } } } diff --git a/apps/desktop/src/plugins/hermes-bots/mcp-setup.tsx b/apps/desktop/src/plugins/hermes-bots/mcp-setup.tsx index 9f864333d6..896a5ecad4 100644 --- a/apps/desktop/src/plugins/hermes-bots/mcp-setup.tsx +++ b/apps/desktop/src/plugins/hermes-bots/mcp-setup.tsx @@ -115,7 +115,7 @@ export function McpSetupButton({ profile, entry, onDone, ensureProfile }: McpSet const [supported, setSupported] = useState(null) const [keyValues, setKeyValues] = useState>({}) const [message, setMessage] = useState('') - const pollRef = useRef | null>(null) + const oauthEpoch = useRef(0) // Holds ONLY the profile this component created on demand. The live prop // wins wherever both exist, so there is nothing to mirror into the ref and // no render of lag between the parent supplying a profile and us using it. @@ -142,8 +142,8 @@ export function McpSetupButton({ profile, entry, onDone, ensureProfile }: McpSet return null } - // eslint-disable-next-line no-restricted-syntax -- clears a timer handle on unmount, not an atom mirror useEffect(() => { + const epoch = oauthEpoch let alive = true mcpSetupSupported().then(ok => { if (alive) { @@ -154,10 +154,7 @@ export function McpSetupButton({ profile, entry, onDone, ensureProfile }: McpSet return () => { alive = false - if (pollRef.current) { - clearInterval(pollRef.current) - pollRef.current = null - } + epoch.current++ } }, []) const isOAuth = (entry.auth || '').toLowerCase() === 'oauth' @@ -248,180 +245,53 @@ export function McpSetupButton({ profile, entry, onDone, ensureProfile }: McpSet } const beginOAuth = async () => { - // A second click (retry, impatient double-click) must not orphan the - // previous poll interval — an overwritten pollRef leaks a 2s poller that - // runs until unmount and can flip phase from a stale OAuth session. - if (pollRef.current) { - clearInterval(pollRef.current) - pollRef.current = null - } + const epoch = ++oauthEpoch.current + + const source = + profile && typeof profile === 'object' + ? { ...profile } + : { connectionId: host.state.connectionId.get(), profile: profile || host.state.profile.get() } setPhase('busy') setMessage('') - const profile = await resolveProfile() + const resolvedProfile = await resolveProfile() - if (!profile) { + if (!resolvedProfile) { setPhase('idle') return } - if (entry.fromCatalog && !entry.installed) { - const add = await mcpRpc('mcp.servers.add', { - profile, - name: entry.name, - preset: entry.name + const scope = { + ...source, + profile: typeof resolvedProfile === 'object' ? resolvedProfile.profile : resolvedProfile + } + + try { + setPhase('oauth') + setMessage('Complete sign-in in your browser...') + await host.completeMcpOAuth({ + serverName: entry.name, + profile: scope, + catalogPreset: entry.fromCatalog && !entry.installed ? entry.name : undefined, + cancelled: () => oauthEpoch.current !== epoch }) - if (!add.ok) { - setPhase('error') - setMessage(add.error || 'Could not add server') - + if (oauthEpoch.current !== epoch) { return } - } - // Client-side callback listener (electron/mcp-oauth-callback-ipc.ts): the - // browser always runs on THIS machine, so hosting the OAuth redirect here - // works for local AND remote backends alike. Against a remote backend it - // is the only working flow — the gateway's own 127.0.0.1 listener is on - // the backend host, unreachable from this machine's browser. Falls back - // to the legacy gateway-listener flow when the bridge or the gateway-side - // callback RPC is unavailable (older builds). - const mcpOauthBridge = - typeof window !== 'undefined' && window.hermesDesktop && window.hermesDesktop.mcpOauth - ? window.hermesDesktop.mcpOauth - : null - - let listener: { id: string; redirectUri: string } | null = null - - if (mcpOauthBridge) { - try { - listener = await mcpOauthBridge.listen() - } catch { - listener = null - } - } - - let start = await mcpRpc('mcp.servers.oauth.start', { - profile, - name: entry.name, - ...(listener ? { client_redirect_uri: listener.redirectUri } : {}) - }) - - // Older gateway rejecting the loopback URI shape (or a stale build that - // validates differently): retry once on the legacy gateway-listener path. - if (!start.ok && listener) { - try { - await mcpOauthBridge!.cancel(listener.id) - } catch { - /* listener teardown is best-effort */ - } - - listener = null - start = await mcpRpc('mcp.servers.oauth.start', { - profile, - name: entry.name - }) - } - - const payload = start.result && (start.result.result || start.result) - const authUrl = payload && (payload.auth_url || payload.verification_url) - const sessionId = payload && payload.session_id - - if (!start.ok || !authUrl || !sessionId) { - if (listener) { - try { - await mcpOauthBridge!.cancel(listener.id) - } catch { - /* listener teardown is best-effort */ - } + setPhase('done') + host.notify({ kind: 'success', message: entry.name + ' authenticated' }) + onDone?.() + } catch (error) { + if (oauthEpoch.current !== epoch) { + return } setPhase('error') - setMessage(start.error || 'Could not start OAuth') - - return + setMessage(error instanceof Error ? error.message : String(error)) } - - // With a client listener bound: await the provider redirect here and relay - // code/state to the gateway. Runs concurrently with the status poll below; - // errors surface through the poll (the gateway marks the flow failed). - if (listener) { - const listenerId = listener.id - - void (async () => { - const cb = await mcpOauthBridge!.wait(listenerId) - - if (cb.error === 'cancelled') { - return - } - - const relay = await mcpRpc('mcp.servers.oauth.callback', { - profile, - name: entry.name, - session_id: sessionId, - code: cb.code || undefined, - state: cb.state || undefined, - error: cb.error || undefined - }) - - const rp = relay.result && (relay.result.result || relay.result) - - if (!relay.ok || (rp && rp.ok === false)) { - setPhase('error') - setMessage((rp && rp.error_message) || relay.error || 'OAuth callback relay failed') - } - })() - } - - // Open the auth URL in the native browser, same as provider OAuth. - // TODO(bot-mode-types): the plugin SDK's `host` has no `openExternal`, so this - // branch is dead and the window bridge / window.open fallbacks are the only - // live paths. `ctx.os.openExternal` is the real verb. Kept as-written under - // ts-expect-error, which reports itself as unused the day the SDK grows one. - try { - // @ts-expect-error TODO(bot-mode-types): not on the SDK host, branch is dead - if (host.openExternal) { - // @ts-expect-error TODO(bot-mode-types): not on the SDK host, branch is dead - host.openExternal(authUrl) - } else if (typeof window !== 'undefined' && window.hermesDesktop && window.hermesDesktop.openExternal) { - window.hermesDesktop.openExternal(authUrl) - } else { - window.open(authUrl, '_blank') - } - } catch { - /* fall through to poll; user can open the URL from the toast */ - } - - setPhase('oauth') - setMessage('Complete sign-in in your browser...') - pollRef.current = setInterval(async () => { - const poll = await mcpRpc('mcp.servers.oauth.poll', { - profile, - name: entry.name, - session_id: sessionId - }) - - const pd = poll.result && (poll.result.result || poll.result) - const status = pd && pd.status - - if (status === 'approved') { - clearInterval(pollRef.current!) - pollRef.current = null - setPhase('done') - host.notify({ - kind: 'success', - message: entry.name + ' authenticated' - }) - onDone && onDone() - } else if (status === 'error') { - clearInterval(pollRef.current!) - pollRef.current = null - setPhase('error') - setMessage((pd && pd.error_message) || 'OAuth failed') - } - }, 2000) } if (supported === false) { diff --git a/apps/desktop/src/sdk/index.ts b/apps/desktop/src/sdk/index.ts index e69550dac7..d6cbf207b6 100644 --- a/apps/desktop/src/sdk/index.ts +++ b/apps/desktop/src/sdk/index.ts @@ -21,6 +21,7 @@ import { atom, computed, type ReadableAtom } from 'nanostores' import type { ReactNode } from 'react' +import { capabilityScoped } from '@/api/client' import { PRIMARY_SESSION_VIEW } from '@/app/chat/session-view' import { openSession, type OpenSessionIntent } from '@/app/open-session' import type { ClientSessionState } from '@/app/types' @@ -43,6 +44,7 @@ import { onGatewayEvent } from '@/contrib/events' import { registry } from '@/contrib/registry' import type { WorkspaceMode } from '@/contrib/types' import { deleteProfile, getLogs, getStatus, hermesApi, type HermesGateway } from '@/hermes' +import { completeMcpDesktopOAuth } from '@/lib/mcp-dashboard-oauth' import { $gateway, activeGatewayConnectionId, @@ -637,6 +639,27 @@ export const host = { /** Tail an app log file (`agent` / `errors` / `gateway` / `gui` / …). */ logs: async (...args: Parameters) => getLogs(...args), + /** Complete client-local MCP sign-in for a pinned bot profile, optionally + * installing its catalog entry first. Uses the same OAuth flow as Settings. */ + completeMcpOAuth: async (options: Parameters[0] & { catalogPreset?: string }) => { + const profile = capabilityScoped(options.profile) + + if (options.catalogPreset) { + const added = await requestGatewayForAgent<{ ok?: boolean; error?: string }>( + profile.connectionId ?? null, + profile.profile || 'default', + 'mcp.servers.add', + { name: options.serverName, preset: options.catalogPreset } + ) + + if (!added.ok) { + throw new Error(added.error || 'Could not add server') + } + } + + return completeMcpDesktopOAuth({ ...options, profile }) + }, + /** Navigate the app router (hash routes, e.g. '/command-center?section=system'). */ navigate: (path: string) => { window.location.hash = path.startsWith('#') ? path : `#${path}` diff --git a/apps/desktop/src/store/suggestion-providers/mcp.ts b/apps/desktop/src/store/suggestion-providers/mcp.ts index 64a5b8a07b..fbefc861db 100644 --- a/apps/desktop/src/store/suggestion-providers/mcp.ts +++ b/apps/desktop/src/store/suggestion-providers/mcp.ts @@ -1,12 +1,5 @@ -import { - addMcpServer, - authMcpServer, - cancelMcpOAuthFlow, - getMcpCatalog, - getMcpOAuthFlow, - listMcpServers, - removeMcpServer -} from '@/hermes' +import { capabilityScoped } from '@/api/client' +import { addMcpServer, getMcpCatalog, listMcpServers, removeMcpServer } from '@/hermes' import { translateNow } from '@/i18n' import { completeMcpDesktopOAuth, McpOAuthCancelled } from '@/lib/mcp-dashboard-oauth' import { MCP_DIRECTORY } from '@/lib/mcp-directory' @@ -190,23 +183,22 @@ export function matchSuggestions(text: string, index: KeywordEntry[]): McpMatch[ } async function connect(known: SuggestibleServer, sessionId: string | null, cancelled: () => boolean): Promise { + const oauthScope = capabilityScoped() + try { - await addMcpServer({ name: known.server, url: known.url }) + await addMcpServer({ name: known.server, url: known.url }, oauthScope) try { await completeMcpDesktopOAuth({ serverName: known.server, - start: authMcpServer, - status: getMcpOAuthFlow, - cancelled, - cancel: cancelMcpOAuthFlow, - openExternal: url => window.hermesDesktop.openExternal(url) + profile: oauthScope, + cancelled }) } catch (error) { // Decline/failure means "no server" — roll back the config write // rather than stranding an unauthorized entry (authoritative-write // rule). Best-effort; the primary error wins. - await removeMcpServer(known.server).catch(() => {}) + await removeMcpServer(known.server, oauthScope).catch(() => {}) throw error } diff --git a/apps/desktop/src/store/suggestion-providers/repair.ts b/apps/desktop/src/store/suggestion-providers/repair.ts index 159348d486..8b0c54d6bf 100644 --- a/apps/desktop/src/store/suggestion-providers/repair.ts +++ b/apps/desktop/src/store/suggestion-providers/repair.ts @@ -1,4 +1,4 @@ -import { authMcpServer, cancelMcpOAuthFlow, getMcpOAuthFlow, listMcpServers } from '@/hermes' +import { listMcpServers } from '@/hermes' import { translateNow } from '@/i18n' import { completeMcpDesktopOAuth, McpOAuthCancelled } from '@/lib/mcp-dashboard-oauth' import { prettyName } from '@/lib/text' @@ -40,11 +40,7 @@ async function reconnect(server: string, sessionId: string | null, cancelled: () try { await completeMcpDesktopOAuth({ serverName: server, - start: authMcpServer, - status: getMcpOAuthFlow, - cancelled, - cancel: cancelMcpOAuthFlow, - openExternal: url => window.hermesDesktop.openExternal(url) + cancelled }) // Fresh tokens reach the live session before the pill claims success. diff --git a/tests/tui_gateway/test_mcp_oauth_cancel.py b/tests/tui_gateway/test_mcp_oauth_cancel.py new file mode 100644 index 0000000000..26238f4a38 --- /dev/null +++ b/tests/tui_gateway/test_mcp_oauth_cancel.py @@ -0,0 +1,105 @@ +"""Cancellation wakes the actual callback worker without crossing profile ownership.""" + +import asyncio +import threading + +import pytest + +from tools.mcp_dashboard_oauth import DashboardOAuthFlow +from tui_gateway import mcp_oauth_sessions as sessions + + +@pytest.mark.parametrize("client_redirect", [False, True]) +def test_cancel_is_scoped_idempotent_and_releases_worker( + tmp_path, monkeypatch, client_redirect +): + monkeypatch.setattr(sessions, "_sessions", {}) + finished = threading.Event() + + def worker(session_id, *_args): + flow = sessions._sessions[session_id]["flow"] + try: + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=test" + ) + ) + asyncio.run(flow.wait_for_callback(timeout=10)) + flow.mark_approved() + except RuntimeError as exc: + flow.mark_error(str(exc)) + finally: + flow.mark_worker_done() + finished.set() + + monkeypatch.setattr(sessions, "_worker", worker) + home = str(tmp_path / "origin") + result = sessions.start_flow( + home, + "reports", + {"url": "https://mcp.example"}, + client_redirect_uri="http://127.0.0.1:49152/callback" + if client_redirect + else None, + ) + sid = result["session_id"] + rec = sessions._sessions[sid] + try: + assert ( + sessions.cancel_flow(sid, "reports", str(tmp_path / "other"))["ok"] is False + ) + assert sessions.cancel_flow(sid, "other", home)["ok"] is False + assert rec["flow"].snapshot()["status"] == "authorization_required" + assert sessions.cancel_flow(sid, "reports", home)["ok"] is True + assert finished.wait(5), ( + "cancel must wake the worker, not leave a 5-minute occupied slot" + ) + assert sessions.poll_flow(sid, "reports")["status"] == "error" + assert rec["httpd"] is None + assert sessions.cancel_flow(sid, "reports", home)["ok"] is True + assert ( + sessions.deliver_callback_flow(sid, "reports", code="late", state="test")[ + "ok" + ] + is False + ) + # A new start can take the per-server slot as soon as the old worker exits. + finished.clear() + retry = sessions.start_flow( + home, + "reports", + {"url": "https://mcp.example"}, + client_redirect_uri="http://127.0.0.1:49152/callback", + ) + assert sessions.cancel_flow(retry["session_id"], "reports", home)["ok"] is True + assert finished.wait(5) + finally: + rec["flow"].mark_error("test cleanup") + sessions._shutdown_listener(rec) + finished.wait(5) + + +def test_cancel_does_not_revoke_an_approved_flow(tmp_path, monkeypatch): + home = str(tmp_path) + flow = DashboardOAuthFlow( + "approved", "reports", None, home, "http://127.0.0.1:49152/callback" + ) + flow.mark_approved() + flow.mark_worker_done() + monkeypatch.setattr( + sessions, + "_sessions", + { + "approved": { + "flow": flow, + "server_name": "reports", + "hermes_home": home, + "httpd": None, + } + }, + ) + assert sessions.cancel_flow("approved", "reports", home) == { + "ok": True, + "status": "approved", + } + assert sessions.cancel_flow("missing", "reports", home)["ok"] is False diff --git a/tui_gateway/mcp_oauth_sessions.py b/tui_gateway/mcp_oauth_sessions.py index 69384d28b0..52ba8f06cd 100644 --- a/tui_gateway/mcp_oauth_sessions.py +++ b/tui_gateway/mcp_oauth_sessions.py @@ -237,6 +237,19 @@ def poll_flow(session_id: str, server_name: str) -> Dict[str, Any]: return out +def cancel_flow(session_id: str, server_name: str, hermes_home: str) -> Dict[str, Any]: + """Cancel only the owning profile's flow and release its callback waiter.""" + rec, err = _lookup(session_id, server_name) + if rec is None: + return {"ok": False, "error_message": err} + if rec["hermes_home"] != hermes_home: + return {"ok": False, "error_message": "profile mismatch for session"} + flow = rec["flow"] + flow.mark_error("OAuth cancelled by user") + _shutdown_listener(rec) + return {"ok": True, "status": flow.snapshot()["status"]} + + def deliver_callback_flow( session_id: str, server_name: str, *, code: Optional[str], state: Optional[str], error: Optional[str] = None) -> Dict[str, Any]: diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index c4d2ebe3ed..7db1fe544e 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -1293,6 +1293,14 @@ def _(rid, params: dict) -> dict: return _ok(rid, {"ok": True, **poll(_str_arg(params, "session_id"), _str_arg(params, "name"))}) +@_mcp_rpc("oauth.cancel", _NAME_SESSION) +def _(rid, params: dict) -> dict: + """Cancel a flow owned by the resolved profile, waking its callback worker.""" + home = str(_tools_mod("hermes_constants").get_hermes_home().expanduser().resolve(strict=False)) + cancel = _tools_mod("tui_gateway.mcp_oauth_sessions").cancel_flow + return _ok(rid, cancel(_str_arg(params, "session_id"), _str_arg(params, "name"), home)) + + @_mcp_rpc("oauth.callback", _NAME_SESSION) def _(rid, params: dict) -> dict: """Relay a client-captured redirect (``code``/``state``/``error``) into a ``client_redirect_uri`` flow."""