diff --git a/apps/desktop/package.json b/apps/desktop/package.json index dd472b0d88..52c31b0f2e 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -67,6 +67,7 @@ "@ant-design/icons-svg": "4.5.0", "@fontsource-variable/geist": "^5.3.0", "@fontsource-variable/geist-mono": "^5.3.0", + "@modelcontextprotocol/sdk": "^1.26.0", "@playwright/test": "^1.62.1", "@storybook/react-vite": "^10.5.5", "@types/react": "^19.2.18", diff --git a/apps/desktop/src/main/__tests__/mcp-editor-validation.test.ts b/apps/desktop/src/main/__tests__/mcp-editor-validation.test.ts index 14282b2904..1c01e9b412 100644 --- a/apps/desktop/src/main/__tests__/mcp-editor-validation.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-editor-validation.test.ts @@ -3,6 +3,38 @@ import { describe, it } from 'node:test'; import { validateMcpEditorDraft } from '../../renderer/mcp-editor-validation.js'; describe('MCP editor validation', () => { + it('reports substantive URL and command errors for live first-edit display', () => { + // The page shows every non-presence error on the FIRST edit; these are + // the codes that must therefore exist immediately, not only on save. + assert.deepEqual( + validateMcpEditorDraft({ id: 'a', kind: 'remote', commandLine: '', url: 'http://lan.example/mcp', headers: '' }), + { url: 'insecure-url' }, + ); + assert.deepEqual( + validateMcpEditorDraft({ id: 'a', kind: 'remote', commandLine: '', url: 'not a url', headers: '' }), + { url: 'invalid-url' }, + ); + assert.deepEqual( + validateMcpEditorDraft({ id: 'a', kind: 'stdio', commandLine: 'npx "unterminated', url: '', headers: '' }), + { commandLine: 'unbalanced-quote' }, + ); + }); + + + it('rejects a remote URL with embedded credentials, mirroring the store', () => { + assert.deepEqual( + validateMcpEditorDraft({ + id: 'api', + kind: 'remote', + commandLine: '', + url: 'https://user:pass@example.com/mcp', + headers: '', + }), + { url: 'url-credentials' }, + ); + }); + + it('requires a server id and the selected transport endpoint', () => { assert.deepEqual( validateMcpEditorDraft({ @@ -10,6 +42,7 @@ describe('MCP editor validation', () => { kind: 'stdio', commandLine: '', url: '', + headers: '', }), { id: 'required', commandLine: 'required' }, ); @@ -19,6 +52,7 @@ describe('MCP editor validation', () => { kind: 'remote', commandLine: '', url: ' ', + headers: '', }), { id: 'required', url: 'required' }, ); @@ -31,6 +65,7 @@ describe('MCP editor validation', () => { kind: 'stdio', commandLine: 'npx -y @modelcontextprotocol/server-filesystem "/my folder"', url: '', + headers: '', }), {}, ); @@ -40,6 +75,7 @@ describe('MCP editor validation', () => { kind: 'stdio', commandLine: 'npx "unterminated', url: '', + headers: '', }), { commandLine: 'unbalanced-quote' }, ); @@ -51,11 +87,33 @@ describe('MCP editor validation', () => { kind: 'stdio', commandLine: '""', url: '', + headers: '', }), { commandLine: 'required' }, ); }); + it('rejects an id that would silently overwrite an existing server', () => { + const draft = { + id: ' notion ', + kind: 'stdio', + commandLine: 'npx server', + url: '', + headers: '', + } as const; + assert.deepEqual( + validateMcpEditorDraft(draft, { existingIds: ['notion', 'filesystem'] }), + { id: 'duplicate-id' }, + ); + // Edit mode passes no existingIds — writing over your own id is the + // point of editing. + assert.deepEqual(validateMcpEditorDraft(draft), {}); + assert.deepEqual( + validateMcpEditorDraft(draft, { existingIds: ['filesystem'] }), + {}, + ); + }); + it('accepts only HTTP(S) URLs for remote servers', () => { assert.deepEqual( validateMcpEditorDraft({ @@ -63,6 +121,7 @@ describe('MCP editor validation', () => { kind: 'remote', commandLine: '', url: 'not a url', + headers: '', }), { url: 'invalid-url' }, ); @@ -72,6 +131,7 @@ describe('MCP editor validation', () => { kind: 'remote', commandLine: '', url: 'file:///tmp/server', + headers: '', }), { url: 'invalid-url' }, ); @@ -81,8 +141,60 @@ describe('MCP editor validation', () => { kind: 'remote', commandLine: '', url: 'https://example.com/mcp', + headers: '', }), {}, ); }); + + it('mirrors the store rule: no Authorization header on an OAuth server', () => { + // The dialog has no OAuth field — the block rides the draft opaquely — + // so without this mirror the placeholder invites exactly the header the + // store rejects, and the save bounces as a raw untranslated toast. + const base = { + id: 'notion', + kind: 'remote' as const, + commandLine: '', + url: 'https://mcp.notion.com/mcp', + }; + assert.deepEqual( + validateMcpEditorDraft( + { ...base, headers: 'Authorization=Bearer t\nX-Workspace=w1' }, + { hasOAuth: true }, + ), + { headers: 'oauth-authorization-conflict' }, + ); + // Case-insensitive, like the store's check. + assert.deepEqual( + validateMcpEditorDraft({ ...base, headers: 'authorization=Bearer t' }, { hasOAuth: true }), + { headers: 'oauth-authorization-conflict' }, + ); + // No oauth block → the header is the user's to configure. + assert.deepEqual( + validateMcpEditorDraft({ ...base, headers: 'Authorization=Bearer t' }, {}), + {}, + ); + // OAuth with other headers is fine. + assert.deepEqual( + validateMcpEditorDraft({ ...base, headers: 'X-Workspace=w1' }, { hasOAuth: true }), + {}, + ); + }); + + it('mirrors the store rule: cleartext http only for loopback hosts', () => { + const draft = (url: string) => + validateMcpEditorDraft({ id: 'remote', kind: 'remote', commandLine: '', url, headers: '' }); + assert.deepEqual(draft('http://192.168.1.50:8080/mcp'), { url: 'insecure-url' }); + assert.deepEqual(draft('http://example.com/mcp'), { url: 'insecure-url' }); + // `*.localhost` is no longer a loopback trust root: Node resolves it + // through the system resolver, so its loopback-ness is not guaranteed. + assert.deepEqual(draft('http://dev.localhost/mcp'), { url: 'insecure-url' }); + for (const url of [ + 'http://127.0.0.1:8080/mcp', + 'http://localhost:3000/mcp', + 'http://[::1]:3000/mcp', + ]) { + assert.deepEqual(draft(url), {}, url); + } + }); }); diff --git a/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts b/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts index 1f928ebf68..2d508b6919 100644 --- a/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts @@ -1,7 +1,12 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; import { MCP_CONFIG_VERSION, type McpConfigFile, type McpServerStatus } from '@maka/core/mcp'; -import { registerMcpIpcMain } from '../mcp-ipc-main.js'; +import { McpServerExistsError } from '@maka/storage'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createMcpConfigStore } from '@maka/storage'; +import { createMcpExclusiveLane, registerMcpIpcMain } from '../mcp-ipc-main.js'; test('MCP IPC commits config before publishing capabilities and emitting status', async () => { const handlers = new Map Promise>(); @@ -34,10 +39,18 @@ test('MCP IPC commits config before publishing capabilities and emitting status' }, manager: { cancelConnect: () => { calls.push('cancel'); return true; }, + forgetServerCredentials: async () => { calls.push('forget'); }, sync: async () => { calls.push('sync'); }, statuses: () => [connected], test: async () => ({ ok: true, status: connected, latencyMs: 1 }), }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => connected, + logout: async () => connected, + resumeLogin: async () => undefined, + }, ensureReady: async () => { calls.push('ready'); }, publishCapabilities: async () => { calls.push('publish'); }, onPublicationError: () => { calls.push('publication:error'); }, @@ -60,7 +73,23 @@ test('MCP IPC commits config before publishing capabilities and emitting status' assert.deepEqual(imported.mcpServers, { remote: { url: 'https://example.com/mcp', enabled: false }, }); + // The bulk edit removed `fixture`: its credentials retire BEFORE the + // config write, so a restart in between cannot orphan them. + assert.deepEqual(calls, ['forget', 'store', 'sync', 'emit', 'publish']); + + calls.length = 0; + const add = handlers.get('mcp:add'); + assert.ok(add); + const added = await add({}, 'brave', { command: 'npx' }); + assert.equal(added.status, 'added'); + assert.deepEqual(added.config.mcpServers.brave, { command: 'npx' }); assert.deepEqual(calls, ['store', 'sync', 'emit', 'publish']); + // A taken id comes back as data, not an IPC error, and commits nothing — + // the existence check now fails before the transaction ever reaches the + // credential-erase or write steps. + calls.length = 0; + assert.deepEqual(await add({}, 'brave', { command: 'other' }), { status: 'exists' }); + assert.deepEqual(calls, []); calls.length = 0; const testHandler = handlers.get('mcp:test'); @@ -74,28 +103,19 @@ test('MCP IPC commits config before publishing capabilities and emitting status' assert.ok(cancelInstall); const cancelled = await cancelInstall({}, 'fixture'); assert.equal(cancelled.mcpServers.fixture, undefined); - assert.deepEqual(calls, ['cancel', 'sync', 'emit', 'publish']); + assert.deepEqual(calls, ['cancel', 'forget', 'store', 'sync', 'emit', 'publish']); }); -test('MCP market cancellation waits for an in-flight config write before rolling it back', async () => { +test('MCP remove aborts before touching the config when credential deletion fails', async () => { const handlers = new Map Promise>(); - let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; - let releaseWrite!: () => void; - let markWriteStarted!: () => void; - const writeGate = new Promise((resolve) => { releaseWrite = resolve; }); - const writeStarted = new Promise((resolve) => { markWriteStarted = resolve; }); - const calls: string[] = []; - + let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: { fixture: { command: 'node' } } }; + let removed = false; registerMcpIpcMain({ ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, store: { get: async () => config, transform: async (apply) => { - calls.push('write:start'); - markWriteStarted(); - await writeGate; config = apply(config); - calls.push('write:end'); return config; }, set: async (next) => { config = next; return next; }, @@ -104,91 +124,39 @@ test('MCP market cancellation waits for an in-flight config write before rolling return config; }, remove: async (serverId) => { - calls.push('remove'); - const { [serverId]: _removed, ...mcpServers } = config.mcpServers; + removed = true; + const { [serverId]: _gone, ...mcpServers } = config.mcpServers; config = { version: MCP_CONFIG_VERSION, mcpServers }; return config; }, }, - manager: { - cancelConnect: () => { calls.push('cancel'); return true; }, - sync: async () => { calls.push('sync'); }, - statuses: () => [], - test: async () => { throw new Error('not used'); }, - }, - ensureReady: async () => {}, - publishCapabilities: async () => { calls.push('publish'); }, - onPublicationError: () => { calls.push('publication:error'); }, - emitChanged: () => { calls.push('emit'); }, - }); - - const install = handlers.get('mcp:install'); - const cancelInstall = handlers.get('mcp:cancelInstall'); - assert.ok(install); - assert.ok(cancelInstall); - - const installing = install({}, 'fixture', { command: 'node' }); - await writeStarted; - const cancelling = cancelInstall({}, 'fixture'); - releaseWrite(); - - const [, cancelled] = await Promise.all([installing, cancelling]); - assert.equal(cancelled.mcpServers.fixture, undefined); - assert.equal(config.mcpServers.fixture, undefined); - assert.deepEqual(calls, ['write:start', 'cancel', 'write:end', 'remove', 'sync', 'emit', 'publish']); -}); - -test('MCP config commit is not rolled back by a capability publication failure', async () => { - const handlers = new Map Promise>(); - let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; - const publicationErrors: unknown[] = []; - registerMcpIpcMain({ - ipcMain: { - handle(channel, handler) { - handlers.set(channel, handler as (...args: any[]) => Promise); - }, - }, - store: { - get: async () => config, - transform: async (apply) => { - config = apply(config); - return config; - }, - set: async (next) => { - config = next; - return next; - }, - upsert: async (serverId, server) => { - config = { - version: MCP_CONFIG_VERSION, - mcpServers: { ...config.mcpServers, [serverId]: server }, - }; - return config; - }, - remove: async () => config, - }, manager: { cancelConnect: () => false, + forgetServerCredentials: async () => { throw new Error('credential store unavailable'); }, sync: async () => {}, statuses: () => [], test: async () => { throw new Error('not used'); }, }, - ensureReady: async () => {}, - publishCapabilities: async () => { - throw new Error('Host disconnected'); + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, }, - onPublicationError: (error) => publicationErrors.push(error), - emitChanged() {}, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, }); - const upsert = handlers.get('mcp:upsert'); - assert.ok(upsert); - const committed = await upsert({}, 'fixture', { command: 'node' }); - assert.deepEqual(committed.mcpServers.fixture, { command: 'node' }); - await new Promise((resolve) => setImmediate(resolve)); - assert.deepEqual(publicationErrors.map((error) => (error as Error).message), [ - 'Host disconnected', - ]); + const remove = handlers.get('mcp:remove'); + assert.ok(remove); + await assert.rejects(remove({}, 'fixture'), /credential store unavailable/u); + // The config was never touched: the server stays configured and the + // removal is retryable — no orphaned token for a same-id re-add. + assert.equal(removed, false); + assert.ok(config.mcpServers.fixture); }); test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel from disk', async () => { @@ -211,21 +179,14 @@ test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel }; const synced: McpConfigFile[] = []; registerMcpIpcMain({ - ipcMain: { - handle(channel, handler) { - handlers.set(channel, handler as (...args: any[]) => Promise); - }, - }, + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, store: { get: async () => config, transform: async (apply) => { config = apply(config); return config; }, - set: async (next) => { - config = next; - return next; - }, + set: async (next) => { config = next; return next; }, upsert: async (serverId, server) => { config = { version: MCP_CONFIG_VERSION, mcpServers: { ...config.mcpServers, [serverId]: server } }; return config; @@ -238,13 +199,17 @@ test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel }, manager: { cancelConnect: () => false, - sync: async (next) => { - synced.push(next); - }, + forgetServerCredentials: async () => {}, + sync: async (next) => { synced.push(next); }, statuses: () => [], - test: async () => { - throw new Error('not used'); - }, + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, }, ensureReady: async () => {}, publishCapabilities: async () => {}, @@ -280,7 +245,9 @@ test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel const stored = config.mcpServers.notion; assert.ok(stored && 'url' in stored); assert.equal(stored.oauth?.clientSecret, 'real-secret'); - assert.equal(synced.at(-1)?.mcpServers.notion, stored); + const syncedNotion = synced.at(-1)?.mcpServers.notion; + assert.ok(syncedNotion && 'url' in syncedNotion); + assert.equal(syncedNotion.oauth?.clientSecret, 'real-secret'); const echoed = returned.mcpServers.notion; assert.ok(echoed && 'url' in echoed); assert.notEqual(echoed.oauth?.clientSecret, 'real-secret'); @@ -308,3 +275,522 @@ test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel assert.ok(survivorAfterCancel.oauth?.clientSecret); assert.notEqual(survivorAfterCancel.oauth?.clientSecret, 'real-secret'); }); + +test('MCP market cancellation waits for an in-flight config write before rolling it back', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; + let releaseWrite!: () => void; + let markWriteStarted!: () => void; + const writeGate = new Promise((resolve) => { releaseWrite = resolve; }); + const writeStarted = new Promise((resolve) => { markWriteStarted = resolve; }); + const calls: string[] = []; + + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + transform: async (apply) => { + calls.push('write:start'); + markWriteStarted(); + await writeGate; + config = apply(config); + calls.push('write:end'); + return config; + }, + set: async (next) => { config = next; return next; }, + upsert: async (serverId, server) => { + config = { version: MCP_CONFIG_VERSION, mcpServers: { ...config.mcpServers, [serverId]: server } }; + return config; + }, + remove: async (serverId) => { + calls.push('remove'); + const { [serverId]: _removed, ...mcpServers } = config.mcpServers; + config = { version: MCP_CONFIG_VERSION, mcpServers }; + return config; + }, + }, + manager: { + cancelConnect: () => { calls.push('cancel'); return true; }, + forgetServerCredentials: async () => { calls.push('forget'); }, + sync: async () => { calls.push('sync'); }, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => { calls.push('publish'); }, + onPublicationError: () => { calls.push('publication:error'); }, + emitChanged: () => { calls.push('emit'); }, + }); + + const install = handlers.get('mcp:install'); + const cancelInstall = handlers.get('mcp:cancelInstall'); + assert.ok(install); + assert.ok(cancelInstall); + + // The fake store skips normalizeMcpConfig, so the install config is given + // in its normal form — the real-store variant below covers the + // normalization mismatch. + const installing = install({}, 'fixture', { enabled: true, command: 'node' }); + await writeStarted; + const cancelling = cancelInstall({}, 'fixture'); + releaseWrite(); + + const [, cancelled] = await Promise.all([installing, cancelling]); + assert.equal(cancelled.mcpServers.fixture, undefined); + assert.equal(config.mcpServers.fixture, undefined); + // The cancellation's own removal is a full transaction on the same lane: + // credentials retire first, then the conditional write. + assert.deepEqual(calls, [ + 'write:start', 'cancel', 'write:end', + 'forget', 'write:start', 'write:end', + 'sync', 'emit', 'publish', + ]); +}); + +test('an active login on a secret-bearing server does not veto edits to another server', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + notion: { + url: 'https://mcp.notion.com/mcp', + oauth: { clientId: 'abc', clientSecret: 'real-secret' }, + }, + other: { command: 'node' }, + }, + }; + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + transform: async (apply) => { config = apply(config); return config; }, + set: async (next) => { config = next; return next; }, + upsert: async (_serverId, _server) => config, + remove: async () => config, + }, + manager: { + cancelConnect: () => false, + forgetServerCredentials: async () => {}, + sync: async () => {}, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + // The login owns `notion` for the whole test. + isActive: (serverId: string) => serverId === 'notion', + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const getConfig = handlers.get('mcp:getConfig'); + const setConfig = handlers.get('mcp:setConfig'); + assert.ok(getConfig); + assert.ok(setConfig); + // The renderer edits the REDACTED config: notion comes back carrying the + // clientSecret sentinel. A sentinel-versus-real-value comparison would + // call that a change; the semantic (restored) comparison must not. + const redacted = await getConfig({}); + const next = await setConfig({}, { + ...redacted, + mcpServers: { + ...redacted.mcpServers, + other: { command: 'node', args: ['--verbose'] }, + }, + }); + const other = next.mcpServers.other; + assert.ok(other && 'command' in other); + assert.deepEqual(other.args, ['--verbose']); + const storedNotion = config.mcpServers.notion; + assert.ok(storedNotion && 'url' in storedNotion); + assert.equal(storedNotion.oauth?.clientSecret, 'real-secret'); + + // Actually touching (here: removing) the login-owned server still fails. + const { notion: _gone, ...withoutNotion } = redacted.mcpServers; + await assert.rejects( + setConfig({}, { ...redacted, mcpServers: withoutNotion }), + /login in progress/u, + ); + assert.ok(config.mcpServers.notion); +}); + +test('a URL change retires the old endpoint credentials before the write, and an erase failure aborts it', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: 'https://old.example.com/mcp' } }, + }; + const calls: string[] = []; + let eraseFails = true; + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + transform: async (apply) => { calls.push('write'); config = apply(config); return config; }, + set: async (next) => { config = next; return next; }, + upsert: async (_serverId, _server) => config, + remove: async () => config, + }, + manager: { + cancelConnect: () => false, + forgetServerCredentials: async () => { + calls.push('forget'); + if (eraseFails) throw new Error('credential store unavailable'); + }, + sync: async () => { calls.push('sync'); }, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const upsert = handlers.get('mcp:upsert'); + assert.ok(upsert); + // Erase fails → nothing persists: the old endpoint's credentials cannot + // outlive a committed repoint across a restart. + await assert.rejects( + upsert({}, 'remote', { url: 'https://new.example.com/mcp' }), + /credential store unavailable/u, + ); + assert.deepEqual(calls, ['forget']); + const kept = config.mcpServers.remote; + assert.ok(kept && 'url' in kept); + assert.equal(kept.url, 'https://old.example.com/mcp'); + + // Same repoint with a healthy credential store: erase strictly precedes + // the write. An unchanged-URL upsert afterwards does not erase at all. + eraseFails = false; + calls.length = 0; + await upsert({}, 'remote', { url: 'https://new.example.com/mcp' }); + assert.deepEqual(calls, ['forget', 'write', 'sync']); + calls.length = 0; + await upsert({}, 'remote', { url: 'https://new.example.com/mcp', enabled: false }); + assert.deepEqual(calls, ['write', 'sync']); +}); + +test('cancelling an install rolls back only its own write, never a newer same-id config', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; + let releaseInstallSync!: () => void; + const installSyncGate = new Promise((resolve) => { releaseInstallSync = resolve; }); + let syncs = 0; + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + transform: async (apply) => { config = apply(config); return config; }, + set: async (next) => { config = next; return next; }, + upsert: async (_serverId, _server) => config, + remove: async () => config, + }, + manager: { + cancelConnect: () => { releaseInstallSync(); return true; }, + forgetServerCredentials: async () => {}, + sync: async () => { + syncs += 1; + // Only the install's connect parks; later syncs pass through. + if (syncs === 1) await installSyncGate; + }, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const install = handlers.get('mcp:install'); + const upsert = handlers.get('mcp:upsert'); + const cancelInstall = handlers.get('mcp:cancelInstall'); + assert.ok(install); + assert.ok(upsert); + assert.ok(cancelInstall); + + // The install commits A and parks in its connect; a newer same-id config + // B lands through upsert while it waits. + const installing = install({}, 'x', { command: 'installed-a' }); + await new Promise((resolve) => setImmediate(resolve)); + await upsert({}, 'x', { command: 'newer-b' }); + + const cancelled = await cancelInstall({}, 'x'); + await installing; + + // The cancellation found B where it committed A: it must decline the + // rollback instead of deleting the newer server (and its credentials). + const survivor = config.mcpServers.x; + assert.ok(survivor && 'command' in survivor); + assert.equal(survivor.command, 'newer-b'); + const echoed = cancelled.mcpServers.x; + assert.ok(echoed && 'command' in echoed); + assert.equal(echoed.command, 'newer-b'); +}); + +test('a login claim travels the shared lane and cannot land inside an open transaction', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { x: { url: 'https://example.com/mcp' } }, + }; + const activeLogins = new Set(); + let releaseWrite!: () => void; + let markWriteStarted!: () => void; + const writeGate = new Promise((resolve) => { releaseWrite = resolve; }); + const writeStarted = new Promise((resolve) => { markWriteStarted = resolve; }); + const lane = createMcpExclusiveLane(); + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + transform: async (apply) => { + markWriteStarted(); + await writeGate; + config = apply(config); + return config; + }, + set: async (next) => { config = next; return next; }, + upsert: async (_serverId, _server) => config, + remove: async () => config, + }, + manager: { + cancelConnect: () => false, + forgetServerCredentials: async () => {}, + sync: async () => {}, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: (serverId: string) => activeLogins.has(serverId), + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + exclusiveLane: lane, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const upsert = handlers.get('mcp:upsert'); + assert.ok(upsert); + // A config transaction is mid-flight (its write is parked)… + const updating = upsert({}, 'x', { url: 'https://new.example.com/mcp' }); + await writeStarted; + // …when a login claim arrives through the SAME lane, the way the OAuth + // controller claims. It must queue behind the transaction, not interleave + // between the gate check and the write. + let claimLanded = false; + const claiming = lane(async () => { + activeLogins.add('x'); + claimLanded = true; + }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(claimLanded, false); + + releaseWrite(); + await updating; + await claiming; + assert.equal(claimLanded, true); + + // With the claim landed, the next transaction's in-lane gate refuses. + await assert.rejects( + upsert({}, 'x', { url: 'https://third.example.com/mcp' }), + /login in progress/u, + ); +}); + +test('cancelling an install through the REAL store rolls the entry back despite normalization', async () => { + // The fake stores in this file skip normalizeMcpConfig; the real store + // rebuilds each server (key order, defaulted enabled/transport, WHATWG + // URL) on write. The cancellation's identity check must compare in that + // normal form, or it mismatches its own persisted entry and silently + // keeps the cancelled server installed. + const root = await mkdtemp(join(tmpdir(), 'mcp-ipc-real-')); + try { + const store = createMcpConfigStore(root); + const handlers = new Map Promise>(); + let releaseInstallSync!: () => void; + const installSyncGate = new Promise((resolve) => { releaseInstallSync = resolve; }); + let syncs = 0; + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store, + manager: { + cancelConnect: () => { releaseInstallSync(); return true; }, + forgetServerCredentials: async () => {}, + sync: async () => { + syncs += 1; + if (syncs === 1) await installSyncGate; + }, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const install = handlers.get('mcp:install'); + const cancelInstall = handlers.get('mcp:cancelInstall'); + assert.ok(install); + assert.ok(cancelInstall); + + // No `enabled`, no `transport`: the store materializes both on write. + const installing = install({}, 'market', { url: 'https://mcp.vercel.com' }); + await new Promise((resolve) => setImmediate(resolve)); + const cancelled = await cancelInstall({}, 'market'); + await installing; + + assert.equal(cancelled.mcpServers.market, undefined); + assert.equal((await store.get()).mcpServers.market, undefined); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('the config write fails closed when the snapshot drifts under the transaction', async () => { + const handlers = new Map Promise>(); + const config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; + const drifted: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { intruder: { command: 'node' } }, + }; + let wrote = false; + registerMcpIpcMain({ + ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, + store: { + get: async () => config, + // Simulates an out-of-band writer landing between the snapshot read + // and the serialized write: apply() observes a different config. + transform: async (apply) => { + const next = apply(drifted); + wrote = true; + return next; + }, + set: async (next) => next, + upsert: async (_serverId, _server) => config, + remove: async () => config, + }, + manager: { + cancelConnect: () => false, + forgetServerCredentials: async () => {}, + sync: async () => {}, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const upsert = handlers.get('mcp:upsert'); + assert.ok(upsert); + await assert.rejects(upsert({}, 'fixture', { command: 'node' }), /changed while/u); + assert.equal(wrote, false); +}); + +test('MCP config commit is not rolled back by a capability publication failure', async () => { + const handlers = new Map Promise>(); + let config: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: {} }; + const publicationErrors: unknown[] = []; + registerMcpIpcMain({ + ipcMain: { + handle(channel, handler) { + handlers.set(channel, handler as (...args: any[]) => Promise); + }, + }, + store: { + get: async () => config, + transform: async (apply) => { + config = apply(config); + return config; + }, + set: async (next) => { + config = next; + return next; + }, + upsert: async (serverId, server) => { + config = { version: MCP_CONFIG_VERSION, mcpServers: { ...config.mcpServers, [serverId]: server } }; + return config; + }, + remove: async () => config, + }, + manager: { + cancelConnect: () => false, + forgetServerCredentials: async () => {}, + sync: async () => {}, + statuses: () => [], + test: async () => { throw new Error('not used'); }, + }, + oauth: { + isActive: () => false, + cancelLogin: () => false, + login: async () => { throw new Error('not used'); }, + logout: async () => { throw new Error('not used'); }, + resumeLogin: async () => undefined, + }, + ensureReady: async () => {}, + publishCapabilities: async () => { + throw new Error('Host disconnected'); + }, + onPublicationError: (error) => publicationErrors.push(error), + emitChanged() {}, + }); + + const upsert = handlers.get('mcp:upsert'); + assert.ok(upsert); + const committed = await upsert({}, 'fixture', { command: 'node' }); + assert.deepEqual(committed.mcpServers.fixture, { command: 'node' }); + await new Promise((resolve) => setImmediate(resolve)); + assert.deepEqual(publicationErrors.map((error) => (error as Error).message), [ + 'Host disconnected', + ]); +}); diff --git a/apps/desktop/src/main/__tests__/mcp-oauth-controller.test.ts b/apps/desktop/src/main/__tests__/mcp-oauth-controller.test.ts new file mode 100644 index 0000000000..029c548bfb --- /dev/null +++ b/apps/desktop/src/main/__tests__/mcp-oauth-controller.test.ts @@ -0,0 +1,747 @@ +import assert from 'node:assert/strict'; +import { createHash, randomUUID } from 'node:crypto'; +import { createServer, type IncomingMessage, type ServerResponse } from 'node:http'; +import { afterEach, test } from 'node:test'; +import { Server as McpServer } from '@modelcontextprotocol/sdk/server/index.js'; +import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; +import { CallToolRequestSchema, ListToolsRequestSchema } from '@modelcontextprotocol/sdk/types.js'; +import { MCP_CONFIG_VERSION } from '@maka/core/mcp'; +import { createMemoryMcpOAuthStorage, McpClientManager } from '@maka/mcp'; +import { createMcpOAuthController } from '../mcp-oauth-controller.js'; + +// The whole desktop login path with a real manager, a real OAuth fixture and +// the real loopback callback listener. The only substitution is the browser: +// openExternal fetches the authorization URL and follows the 302 back to the +// 127.0.0.1 callback — exactly the two requests a real browser would make. + +const cleanups: Array<() => Promise> = []; +afterEach(async () => { + await Promise.all(cleanups.splice(0).map((cleanup) => cleanup())); +}); + +test('controller login drives browser round-trip to a connected server', async () => { + const fixture = await createOAuthFixture(); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + cleanups.push(() => manager.close()); + + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + + const browserVisits: string[] = []; + const controller = createMcpOAuthController({ + manager, + openExternal: async (url) => { + browserVisits.push(url); + // A browser: load the consent screen, then follow its redirect. + const consent = await fetch(url, { redirect: 'manual' }); + assert.equal(consent.status, 302); + const callback = await fetch(consent.headers.get('location') ?? ''); + assert.equal(callback.status, 200); + }, + }); + + const status = await controller.login('remote'); + assert.equal(status.state, 'connected'); + assert.equal(status.authenticated, true); + assert.equal(browserVisits.length, 1); + // The callback listener bound an ephemeral loopback port and the fixture + // redirected straight into it. + assert.match(new URL(browserVisits[0] ?? '').searchParams.get('redirect_uri') ?? '', /^http:\/\/127\.0\.0\.1:\d+\/callback$/u); + + const echoBinding = manager + .toolSnapshot() + .tools.find( + (tool) => tool.descriptor.serverId === 'remote' && tool.descriptor.name === 'echo', + )?.binding; + assert.ok(echoBinding); + const echo = await manager.callTool(echoBinding, { value: 'via-controller' }); + assert.deepEqual(echo.content, [{ type: 'text', text: 'via-controller' }]); + + const after = await controller.logout('remote'); + assert.equal(after.state, 'needs-auth'); +}); + +test('resumeLogin rebinds the persisted callback port and completes the round', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + cleanups.push(() => manager.close()); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + + // "First run": a login starts — verifier, state and the callback port are + // persisted — and the app dies before the browser returns. + const port = await freeLoopbackPort(); + const start = await manager.startAuthorization('remote', `http://127.0.0.1:${port}/callback`, { + state: 'resume-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + + // "Second run": the controller rebinds the persisted port from storage. + const controller = createMcpOAuthController({ manager, openExternal: async () => {} }); + const resumed = controller.resumeLogin('remote'); + + // The user's browser finishes the round it had already started. + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + assert.equal(consent.status, 302); + const location = consent.headers.get('location'); + assert.ok(location); + const callback = await fetchWithRetry(location); + assert.equal(callback.status, 200); + + const status = await resumed; + assert.equal(status?.state, 'connected'); + assert.equal(status?.authenticated, true); + + // Nothing left to resume once the round settled. + assert.equal(await controller.resumeLogin('remote'), undefined); +}); + +test('resumeLogin resolves undefined when the persisted port is already taken', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + cleanups.push(() => manager.close()); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + + const port = await freeLoopbackPort(); + const start = await manager.startAuthorization('remote', `http://127.0.0.1:${port}/callback`, { + state: 'occupied-port-state', + }); + assert.equal(start.status, 'redirect'); + + // Something else grabbed the port before the restart's resume ran. + const squatter = createServer(); + await new Promise((resolve, reject) => { + squatter.once('error', reject); + squatter.listen(port, '127.0.0.1', resolve); + }); + try { + const controller = createMcpOAuthController({ manager, openExternal: async () => {} }); + // Per contract this is "nothing to resume", not a failure. + assert.equal(await controller.resumeLogin('remote'), undefined); + } finally { + squatter.closeAllConnections(); + await new Promise((resolve) => squatter.close(() => resolve())); + } +}); + +test('login refuses a cleartext non-loopback authorization URL without opening it', async () => { + const opened: string[] = []; + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => ({ + status: 'redirect', + state: 's', + issuer: 'https://as.example', + scopes: ['files:read'], + authorizationUrl: 'http://as.example.com/authorize', + }), + finishAuthorization: async () => { + throw new Error('unreachable'); + }, + clearAuthorization: async () => { + throw new Error('unreachable'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async (url) => { + opened.push(url); + }, + }); + await assert.rejects(controller.login('remote'), /https/u); + assert.deepEqual(opened, []); +}); + +test('a reflected error_description never crosses into the login rejection', async () => { + const fixture = await createOAuthFixture({ + authorizeError: 'access_denied', + authorizeErrorDescription: 'leak token-echo-abcdef', + }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + cleanups.push(() => manager.close()); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + + const controller = createMcpOAuthController({ + manager, + openExternal: async (url) => { + const consent = await fetch(url, { redirect: 'manual' }); + const location = consent.headers.get('location'); + assert.ok(location); + await fetch(location); + }, + }); + + await assert.rejects(controller.login('remote'), (error: unknown) => { + assert.ok(error instanceof Error); + // The description is the server's arbitrary string; only the registered + // error code may cross toward the renderer. + assert.doesNotMatch(error.message, /token-echo-abcdef/u); + assert.match(error.message, /access_denied/u); + return true; + }); +}); + +test('an unregistered callback error code is generalized before crossing IPC', async () => { + const fixture = await createOAuthFixture({ authorizeError: 'opaqueSecret123' }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + cleanups.push(() => manager.close()); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + + const controller = createMcpOAuthController({ + manager, + openExternal: async (url) => { + const consent = await fetch(url, { redirect: 'manual' }); + const location = consent.headers.get('location'); + assert.ok(location); + await fetch(location); + }, + }); + + await assert.rejects(controller.login('remote'), (error: unknown) => { + assert.ok(error instanceof Error); + // Only allowlisted RFC 6749 codes may cross; an attacker-shaped code + // that merely looks like an identifier must not tunnel through. + assert.doesNotMatch(error.message, /opaqueSecret123/u); + assert.match(error.message, /unknown_error/u); + return true; + }); +}); + +test('controller rejects a forged callback state and an OAuth error response', async () => { + const fixture = await createOAuthFixture({ authorizeError: 'access_denied' }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + cleanups.push(() => manager.close()); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url: fixture.mcpUrl, transport: 'streamable-http' } }, + }); + + const controller = createMcpOAuthController({ + manager, + openExternal: async (url) => { + const authorization = new URL(url); + const redirectUri = new URL(authorization.searchParams.get('redirect_uri') ?? ''); + // A forged state must be rejected without settling the login... + redirectUri.searchParams.set('code', 'forged'); + redirectUri.searchParams.set('state', 'wrong'); + const forged = await fetch(redirectUri); + assert.equal(forged.status, 400); + // ...and then the real consent screen reports the user's refusal. + const consent = await fetch(url, { redirect: 'manual' }); + const location = consent.headers.get('location'); + assert.ok(location); + await fetch(location); + }, + }); + + await assert.rejects(controller.login('remote'), /access_denied|Authorization failed/u); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + // The denied round is terminally dead: its persisted verifier/state are + // abandoned, so nothing resumes it after a restart (and the login guard + // is not re-occupied on every boot). + assert.equal(await manager.pendingAuthorization('remote'), undefined); + assert.equal(await controller.resumeLogin('remote'), undefined); +}); + +test('a hung readiness gate or store lookup cannot outlive the round deadline', async () => { + // Preflight is controller-owned and deadline-raced: a wedged ensureReady + // or credential/config store read must not park the promise (and the + // renderer's per-server login lock) forever. + for (const wedge of ['ready', 'store'] as const) { + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => { + throw new Error('preflight must fail first'); + }, + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + throw new Error('the browser must not open'); + }, + ensureReady: wedge === 'ready' ? () => new Promise(() => {}) : undefined, + callbackPort: wedge === 'store' ? () => new Promise(() => {}) : undefined, + loginTimeoutMs: 60, + }); + await assert.rejects(controller.login('remote'), /Timed out/u); + // Guard released: the retry runs instead of "already in progress". + await assert.rejects(controller.login('remote'), /Timed out/u); + } +}); + +test('cancelling a round releases the guard and clears the pending state', async () => { + // "Clicked Login, closed the tab" must not be a five-minute trap in + // which every config edit for the server is vetoed: cancel ends the + // round like a timeout — rejection, guard release, terminal cleanup. + const abandoned: string[] = []; + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => ({ + status: 'redirect' as const, + authorizationUrl: 'https://as.example/authorize', + state: 's', + issuer: 'https://as.example', + scopes: ['files:read'], + }), + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + abandonAuthorization: async (serverId) => { + abandoned.push(serverId); + }, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + // The browser opens and the callback never arrives. + openExternal: async () => {}, + loginTimeoutMs: 60_000, + }); + + const login = controller.login('remote'); + login.catch(() => {}); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(controller.isActive('remote'), true); + + assert.equal(controller.cancelLogin('remote'), true); + await assert.rejects(login, /cancelled/u); + assert.equal(controller.isActive('remote'), false); + assert.deepEqual(abandoned, ['remote']); + // No round → nothing to cancel. + assert.equal(controller.cancelLogin('remote'), false); +}); + +test('a hung terminal-failure cleanup cannot park the login rejection', async () => { + // The abandon often shares the exact resource that stalled the round (a + // wedged credential lane): awaiting it unbounded would hold the rejection + // — and the renderer's login lock — forever. + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => { + throw new Error('authorization endpoint refused'); + }, + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + // The cleanup itself never settles. + abandonAuthorization: () => new Promise(() => {}), + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + throw new Error('the browser must not open'); + }, + loginTimeoutMs: 60, + }); + await assert.rejects(controller.login('remote'), /authorization endpoint refused/u); + // The guard released with the bounded cleanup: a retry runs. + await assert.rejects(controller.login('remote'), /authorization endpoint refused/u); +}); + +test('a timed-out logout aborts the in-flight credential clear', async () => { + // Racing alone would abandon the caller while the stalled clear kept + // running — free to resume later and tombstone the fresh tokens a newer + // login stored. The deadline's signal must travel INTO the clear. + const seen: Array = []; + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => { + throw new Error('not used'); + }, + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: (_serverId, options) => { + seen.push(options?.signal); + return new Promise(() => {}); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + throw new Error('the browser must not open'); + }, + loginTimeoutMs: 60, + }); + await assert.rejects(controller.logout('remote'), /Timed out/u); + assert.equal(seen.length, 1); + assert.ok(seen[0]); + assert.equal(seen[0]?.aborted, true); +}); + +test('a hung readiness gate or credential clear cannot outlive the logout deadline', async () => { + // Logout is bounded like login: a wedged ensureReady or a hung + // clearAuthorization (store erase / reconnect) must not park the + // renderer's logout lock forever. + for (const wedge of ['ready', 'clear'] as const) { + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => { + throw new Error('not used'); + }, + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: + wedge === 'clear' + ? () => new Promise(() => {}) + : async () => { + throw new Error('readiness must fail first'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + throw new Error('the browser must not open'); + }, + ensureReady: wedge === 'ready' ? () => new Promise(() => {}) : undefined, + loginTimeoutMs: 60, + }); + await assert.rejects(controller.logout('remote'), /Timed out/u); + } +}); + +test('a hung browser launch cannot outlive the round deadline', async () => { + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async () => ({ + status: 'redirect' as const, + state: 's', + issuer: 'https://as.example', + scopes: ['files:read'], + authorizationUrl: 'https://as.example/authorize', + }), + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + // The shell accepts the launch request and never settles. + openExternal: () => new Promise(() => {}), + loginTimeoutMs: 100, + }); + await assert.rejects(controller.login('remote'), /Timed out/u); + // The guard released with the round; a retry is not "already in progress". + await assert.rejects(controller.login('remote'), /Timed out/u); +}); + +test('a hung discovery cannot hold the login guard past the deadline', async () => { + // The deadline covers the whole round: a metadata endpoint that accepts + // the connection and never answers must not park the login forever. + let starts = 0; + const roundSignals: Array = []; + const controller = createMcpOAuthController({ + manager: { + startAuthorization: (_serverId, _redirectUrl, options) => { + starts += 1; + roundSignals.push(options?.signal); + return new Promise(() => {}); + }, + finishAuthorization: async () => { + throw new Error('not used'); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + throw new Error('the browser must not open for a hung discovery'); + }, + loginTimeoutMs: 50, + }); + await assert.rejects(controller.login('remote'), /Timed out/u); + // The in-progress guard released with the round — a retry starts cleanly + // instead of being refused as already in progress. + await assert.rejects(controller.login('remote'), /Timed out/u); + assert.equal(starts, 2); + // The timeout did not merely abandon the caller: each round's underlying + // flow received the deadline's signal and was aborted with it, so a late + // completion cannot write over a newer round (the manager fences every + // storage write on this signal). + assert.equal(roundSignals.length, 2); + for (const signal of roundSignals) { + assert.ok(signal); + assert.equal(signal.aborted, true); + } +}); + +test('a hung token endpoint releases the listener and the guard at the deadline', async () => { + let capturedRedirect = ''; + let capturedState = ''; + let finishSignal: AbortSignal | undefined; + const controller = createMcpOAuthController({ + manager: { + startAuthorization: async (_serverId, redirectUrl, options) => { + capturedRedirect = redirectUrl; + capturedState = options?.state ?? ''; + return { + status: 'redirect' as const, + authorizationUrl: 'https://as.example/authorize', + state: 's', + issuer: 'https://as.example', + scopes: ['files:read'], + }; + }, + // The token exchange hangs: connection accepted, response never sent. + finishAuthorization: (_serverId, _callback, options) => { + finishSignal = options?.signal; + return new Promise(() => {}); + }, + clearAuthorization: async () => { + throw new Error('not used'); + }, + abandonAuthorization: async () => {}, + pendingAuthorization: async () => undefined, + status: () => undefined, + }, + openExternal: async () => { + // The browser round itself completes fine… + const callback = new URL(capturedRedirect); + callback.searchParams.set('state', capturedState); + callback.searchParams.set('code', 'hung-code'); + const response = await fetch(callback); + assert.equal(response.status, 200); + }, + // Generous enough that the real loopback fetch in openExternal cannot + // eat the budget on a stalled runner; the hung exchange still dominates. + loginTimeoutMs: 750, + }); + // …and the round still times out on the exchange. + await assert.rejects(controller.login('remote'), /Timed out/u); + // The loopback listener closed with the round; the port is released. + await assert.rejects(fetch(capturedRedirect)); + // The hung exchange was aborted with the round, so its late completion + // cannot land writes over a newer round. + assert.equal(finishSignal?.aborted, true); +}); + +interface OAuthFixture { + mcpUrl: string; +} + +/** An OS-assigned port that is free right now — bound briefly, then + * released for the resume flow to claim. */ +async function freeLoopbackPort(): Promise { + const probe = createServer(); + await new Promise((resolve, reject) => { + probe.once('error', reject); + probe.listen(0, '127.0.0.1', resolve); + }); + const address = probe.address(); + if (!address || typeof address === 'string') throw new Error('probe did not bind'); + const port = address.port; + await new Promise((resolve) => probe.close(() => resolve())); + return port; +} + +/** The resumed listener binds asynchronously; retry briefly so the + * browser's callback does not race it. */ +async function fetchWithRetry(url: string): Promise { + let lastError: unknown; + for (let attempt = 0; attempt < 40; attempt += 1) { + try { + return await fetch(url); + } catch (error) { + lastError = error; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + } + throw lastError; +} + +async function createOAuthFixture( + options: { authorizeError?: string; authorizeErrorDescription?: string } = {}, +): Promise { + const accessToken = `token-${randomUUID()}`; + const pendingCodes = new Map(); + let origin = ''; + + const httpServer = createServer(async (req, res) => { + const url = new URL(req.url ?? '/', origin); + try { + if (url.pathname === '/mcp' && req.method === 'POST') { + if (req.headers.authorization !== `Bearer ${accessToken}`) { + res + .writeHead(401, { + 'content-type': 'application/json', + 'www-authenticate': `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource"`, + }) + .end(JSON.stringify({ error: 'unauthorized' })); + return; + } + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: undefined }); + const server = createProtocolServer(); + await server.connect(transport); + res.once('close', () => { + void transport.close(); + void server.close(); + }); + await transport.handleRequest(req, res, await readJsonBody(req)); + return; + } + if (url.pathname === '/.well-known/oauth-protected-resource') { + json(res, { resource: `${origin}/mcp`, authorization_servers: [origin] }); + return; + } + if (url.pathname === '/.well-known/oauth-authorization-server') { + json(res, { + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + token_endpoint: `${origin}/token`, + registration_endpoint: `${origin}/register`, + response_types_supported: ['code'], + grant_types_supported: ['authorization_code', 'refresh_token'], + code_challenge_methods_supported: ['S256'], + token_endpoint_auth_methods_supported: ['none'], + }); + return; + } + if (url.pathname === '/register' && req.method === 'POST') { + const body = (await readJsonBody(req)) as Record; + json(res, { + client_id: `client-${randomUUID()}`, + redirect_uris: body.redirect_uris, + token_endpoint_auth_method: 'none', + }); + return; + } + if (url.pathname === '/authorize') { + const redirectUri = url.searchParams.get('redirect_uri'); + const state = url.searchParams.get('state'); + const challenge = url.searchParams.get('code_challenge'); + if (!redirectUri || !challenge) { + res.writeHead(400).end('missing parameters'); + return; + } + const target = new URL(redirectUri); + if (options.authorizeError) { + target.searchParams.set('error', options.authorizeError); + if (options.authorizeErrorDescription) { + target.searchParams.set('error_description', options.authorizeErrorDescription); + } + } else { + const code = `code-${randomUUID()}`; + pendingCodes.set(code, { challenge }); + target.searchParams.set('code', code); + } + if (state) target.searchParams.set('state', state); + res.writeHead(302, { location: target.toString() }).end(); + return; + } + if (url.pathname === '/token' && req.method === 'POST') { + const params = new URLSearchParams(await readTextBody(req)); + const pending = pendingCodes.get(params.get('code') ?? ''); + const hashed = createHash('sha256') + .update(params.get('code_verifier') ?? '') + .digest('base64url'); + if (!pending || hashed !== pending.challenge) { + json(res, { error: 'invalid_grant' }, 400); + return; + } + json(res, { access_token: accessToken, token_type: 'Bearer', expires_in: 3600 }); + return; + } + res.writeHead(404).end(); + } catch (error) { + if (!res.headersSent) res.writeHead(500); + res.end(error instanceof Error ? error.message : String(error)); + } + }); + + await new Promise((resolve, reject) => { + httpServer.once('error', reject); + httpServer.listen(0, '127.0.0.1', resolve); + }); + const address = httpServer.address(); + if (!address || typeof address === 'string') throw new Error('OAuth fixture did not bind TCP'); + origin = `http://127.0.0.1:${address.port}`; + cleanups.push( + () => + new Promise((resolve, reject) => { + httpServer.closeAllConnections(); + httpServer.close((error) => (error ? reject(error) : resolve())); + }), + ); + return { mcpUrl: `${origin}/mcp` }; +} + +function createProtocolServer(): McpServer { + const server = new McpServer( + { name: 'maka-oauth-controller-fixture', version: '1.0.0' }, + { capabilities: { tools: {} } }, + ); + server.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: [ + { + name: 'echo', + description: 'Echo text', + inputSchema: { type: 'object', properties: { value: { type: 'string' } } }, + }, + ], + })); + server.setRequestHandler(CallToolRequestSchema, async ({ params }) => ({ + content: [{ type: 'text', text: String(params.arguments?.value ?? '') }], + })); + return server; +} + +function json(res: ServerResponse, body: unknown, status = 200): void { + res.writeHead(status, { 'content-type': 'application/json' }).end(JSON.stringify(body)); +} + +async function readJsonBody(req: IncomingMessage): Promise { + const text = await readTextBody(req); + return text ? JSON.parse(text) : undefined; +} + +function readTextBody(req: IncomingMessage): Promise { + return new Promise((resolve, reject) => { + let data = ''; + req.on('data', (chunk) => { + data += String(chunk); + }); + req.once('end', () => resolve(data)); + req.once('error', reject); + }); +} diff --git a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts new file mode 100644 index 0000000000..85579f6083 --- /dev/null +++ b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts @@ -0,0 +1,29 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { test } from 'node:test'; + +// The MCP IPC handlers are registered on the Runtime Host's ScopedIpcMain, +// whose first argument must be a DesktopHostRef. A raw ipcRenderer.invoke +// would put serverId in that slot and fail requireDesktopHostRef before the +// handler ever ran — the bug this contract test pins down at the source +// level, since the preload itself only runs inside Electron. +const preloadSource = readFileSync( + fileURLToPath(new URL('../../../src/preload/preload.ts', import.meta.url)), + 'utf8', +); + +test('every MCP bridge method rides the scoped Runtime Host seam', () => { + const rawMcpInvokes = preloadSource.match(/ipcRenderer\.invoke\(\s*'mcp:/gu) ?? []; + assert.deepEqual(rawMcpInvokes, []); + for (const channel of [ + 'mcp:getConfig', + 'mcp:add', + 'mcp:upsert', + 'mcp:remove', + 'mcp:login', + 'mcp:logout', + ]) { + assert.match(preloadSource, new RegExp(`invokeActiveRuntimeHost\\('${channel}'`, 'u')); + } +}); diff --git a/apps/desktop/src/main/mcp-ipc-main.ts b/apps/desktop/src/main/mcp-ipc-main.ts index 226e1657bc..fcf9f848ab 100644 --- a/apps/desktop/src/main/mcp-ipc-main.ts +++ b/apps/desktop/src/main/mcp-ipc-main.ts @@ -1,7 +1,15 @@ import type { IpcMain } from 'electron'; -import type { McpConfigFile, McpServerConfig, McpServerStatus } from '@maka/core/mcp'; +import { + MCP_CONFIG_VERSION, + isMcpStdioConfig, + type McpConfigAddResult, + type McpConfigFile, + type McpServerConfig, + type McpServerStatus, +} from '@maka/core/mcp'; import type { McpClientManager } from '@maka/mcp'; -import type { McpConfigStore } from '@maka/storage'; +import { McpServerExistsError, normalizeMcpConfig, type McpConfigStore } from '@maka/storage'; +import type { McpOAuthController } from './mcp-oauth-controller.js'; import { redactMcpConfigSecrets, restoreMcpConfigSecrets, @@ -11,15 +19,101 @@ import { export interface McpIpcMainDeps { ipcMain: Pick; store: McpConfigStore; - manager: Pick; + manager: Pick< + McpClientManager, + 'sync' | 'statuses' | 'test' | 'cancelConnect' | 'forgetServerCredentials' + >; + oauth: McpOAuthController; + /** Shared with the OAuth controller (see createMcpExclusiveLane). Falls + * back to a private lane when not provided. */ + exclusiveLane?: McpExclusiveLane; ensureReady(): Promise; publishCapabilities(): Promise; onPublicationError(error: unknown): void; emitChanged(statuses: McpServerStatus[]): void; } +/** One serialized slot at a time. Config transactions AND the OAuth + * controller's login claims run through the same lane, so "no login is + * active" checked inside a transaction cannot be invalidated by a claim + * landing between the check and the write — and a claim never lands while + * a transaction is mid-flight. */ +export type McpExclusiveLane = (work: () => Promise) => Promise; + +export function createMcpExclusiveLane(): McpExclusiveLane { + let lane: Promise = Promise.resolve(); + return (work) => { + const run = lane.then(work, work); + lane = run.then( + () => undefined, + () => undefined, + ); + return run; + }; +} + export function registerMcpIpcMain(deps: McpIpcMainDeps): void { - const installs = new Map; settle(): void }>(); + const installs = new Map< + string, + { cancelled: boolean; committed?: string; settled: Promise; settle(): void } + >(); + // Main is the authority on operation exclusivity, not the renderer's + // advisory locks: while a login round owns a server, a config mutation + // would race the browser callback against a changed or absent server. + const assertNoActiveLogin = (serverId: string) => { + if (deps.oauth.isActive(serverId)) { + throw new Error( + `MCP server "${serverId}" has a login in progress — wait for it to finish before changing its configuration`, + ); + } + }; + // Every config mutation is one transaction on one lane: + // read the authoritative snapshot → restore sentinels and apply the + // active-login gate against it → erase the credentials this commit + // orphans (removed servers, repointed endpoints) → persist. + // The credential erasure is asynchronous, so it cannot live inside the + // store's synchronous transform; the lane serializes the whole sequence + // instead, and the final transform still fails closed if the snapshot + // drifted under an out-of-band writer. Credentials go first so a failed + // erase aborts the commit while everything is still configured and + // retryable — never a persisted removal whose token a same-id re-add + // could inherit after a restart. + const inMutationLane = deps.exclusiveLane ?? createMcpExclusiveLane(); + const commitConfig = async ( + mutate: (current: McpConfigFile) => McpConfigFile, + ): Promise => { + const current = await deps.store.get(); + const next = mutate(current); + // The authoritative gate: every server this commit semantically touches + // is re-checked INSIDE the lane. The handler-entry checks are advisory + // fast-fails; this one cannot race a login claim, because claims travel + // the same lane. + for (const serverId of new Set([ + ...Object.keys(current.mcpServers), + ...Object.keys(next.mcpServers), + ])) { + const before = current.mcpServers[serverId]; + const after = next.mcpServers[serverId]; + if (JSON.stringify(before) !== JSON.stringify(after)) assertNoActiveLogin(serverId); + } + // Erases are per-server and not transactional as a set: if one fails + // partway, the commit aborts with the EARLIER servers already logged + // out. That partial effect is deliberately in the fail-closed direction + // — a re-login is recoverable, a credential outliving its removed or + // repointed config is not. + for (const serverId of credentialRetirements(current, next)) { + await deps.manager.forgetServerCredentials(serverId); + } + const snapshot = JSON.stringify(current); + return deps.store.transform((actual) => { + if (JSON.stringify(actual) !== snapshot) { + throw new Error( + 'MCP configuration changed while this update was being prepared — retry the operation', + ); + } + return next; + }); + }; // The renderer is semi-trusted (SECURITY.md §3): every config that crosses // toward it leaves with clientSecret replaced by the sentinel, and every // config it sends back has sentinels restored from disk before the store @@ -32,48 +126,102 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { await deps.ensureReady(); return deps.manager.statuses(); }); - // Restore runs INSIDE the store's serialized transform: reading a - // snapshot first and writing later would let a concurrent update commit a - // rotated secret in between, and the marker-bearing write would then - // restore the OLD secret over it. - deps.ipcMain.handle('mcp:setConfig', async (_event, config: McpConfigFile) => { - const next = await deps.store.transform((current) => - restoreMcpConfigSecrets(config, current), - ); - await deps.manager.sync(next); - changed(deps); - return redactMcpConfigSecrets(next); - }); + deps.ipcMain.handle('mcp:setConfig', async (_event, config: McpConfigFile) => + inMutationLane(async () => { + // Sentinels restore BEFORE the gate compares anything: the renderer's + // copy of an untouched secret-bearing server still carries sentinels, + // and comparing those against the stored real values would make every + // such server look modified — vetoing a bulk edit that only touches + // an unrelated server while some other login runs. commitConfig's + // in-lane gate does the semantic comparison on the restored config. + const next = await commitConfig((current) => restoreMcpConfigSecrets(config, current)); + await deps.manager.sync(next); + changed(deps); + return redactMcpConfigSecrets(next); + }), + ); + deps.ipcMain.handle( + 'mcp:add', + async (_event, serverId: string, config: McpServerConfig): Promise => { + assertNoActiveLogin(serverId); + try { + const next = await inMutationLane(() => + commitConfig((current) => { + // Existence check and write are one serialized step, and the + // restore reads the same current snapshot the write commits over. + if (Object.hasOwn(current.mcpServers, serverId)) { + throw new McpServerExistsError(serverId); + } + return { + ...current, + mcpServers: { + ...current.mcpServers, + [serverId]: restoreMcpServerSecret(serverId, config, current), + }, + }; + }), + ); + await deps.manager.sync(next); + changed(deps); + return { status: 'added', config: redactMcpConfigSecrets(next) }; + } catch (error) { + if (error instanceof McpServerExistsError) return { status: 'exists' }; + throw error; + } + }, + ); deps.ipcMain.handle('mcp:upsert', async (_event, serverId: string, config: McpServerConfig) => { - const next = await deps.store.transform((current) => ({ - ...current, - mcpServers: { - ...current.mcpServers, - [serverId]: restoreMcpServerSecret(serverId, config, current), - }, - })); + assertNoActiveLogin(serverId); + const next = await inMutationLane(() => + commitConfig((current) => ({ + ...current, + mcpServers: { + ...current.mcpServers, + [serverId]: restoreMcpServerSecret(serverId, config, current), + }, + })), + ); await deps.manager.sync(next); changed(deps); return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:install', async (_event, serverId: string, config: McpServerConfig) => { + assertNoActiveLogin(serverId); if (installs.has(serverId)) throw new Error(`MCP install already in progress: ${serverId}`); let settle!: () => void; const operation = { cancelled: false, + committed: undefined as string | undefined, settled: new Promise((resolve) => { settle = resolve; }), settle: () => settle(), }; installs.set(serverId, operation); try { - const next = await deps.store.transform((current) => ({ - ...current, - mcpServers: { - ...current.mcpServers, - [serverId]: restoreMcpServerSecret(serverId, config, current), - }, - })); + const next = await inMutationLane(() => + commitConfig((current) => { + const installed = restoreMcpServerSecret(serverId, config, current); + // What THIS install committed, for the cancellation to compare + // against: a cancel must only roll back its own write, never a + // newer same-id configuration that landed after it. Recorded in + // the STORE's normal form — the real store normalizes on write + // (key order, defaulted enabled/transport, WHATWG URL), so the + // raw restored shape would mismatch its own persisted entry and + // the rollback would silently no-op. + operation.committed = JSON.stringify( + normalizeMcpConfig({ + version: MCP_CONFIG_VERSION, + mcpServers: { [serverId]: installed }, + }).mcpServers[serverId], + ); + return { + ...current, + mcpServers: { ...current.mcpServers, [serverId]: installed }, + }; + }), + ); if (operation.cancelled) return redactMcpConfigSecrets(next); + // The connect runs OUTSIDE the mutation lane: a cancellation must be + // able to interrupt it, and its own removal transaction needs the lane. try { await deps.manager.sync(next); } catch (error) { @@ -86,8 +234,16 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { operation.settle(); } }); + const removeServer = async (serverId: string): Promise => + inMutationLane(() => + commitConfig((current) => { + const { [serverId]: _removed, ...mcpServers } = current.mcpServers; + return { ...current, mcpServers }; + }), + ); deps.ipcMain.handle('mcp:remove', async (_event, serverId: string) => { - const next = await deps.store.remove(serverId); + assertNoActiveLogin(serverId); + const next = await removeServer(serverId); await deps.manager.sync(next); changed(deps); // Still a full config crossing toward the renderer: the remaining @@ -95,11 +251,26 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:cancelInstall', async (_event, serverId: string) => { + assertNoActiveLogin(serverId); const operation = installs.get(serverId); if (operation) operation.cancelled = true; deps.manager.cancelConnect(serverId); await operation?.settled; - const next = await deps.store.remove(serverId); + // Roll back only the install's OWN write. While the cancel waited, an + // upsert can have replaced the entry with a newer same-id config — + // removing whatever is current would delete that newer server and + // retire its credentials. + const next = await inMutationLane(() => + commitConfig((current) => { + const entry = current.mcpServers[serverId]; + if (entry === undefined) return current; + if (operation?.committed !== undefined && JSON.stringify(entry) !== operation.committed) { + return current; + } + const { [serverId]: _removed, ...mcpServers } = current.mcpServers; + return { ...current, mcpServers }; + }), + ); await deps.manager.sync(next); changed(deps); return redactMcpConfigSecrets(next); @@ -110,6 +281,57 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { deps.emitChanged(deps.manager.statuses()); return result; }); + deps.ipcMain.handle('mcp:login', async (_event, serverId: string) => { + // No preflight here: readiness and the callback-port lookup run INSIDE + // the controller under its round deadline, so a stalled store cannot + // park this promise (and the renderer's login lock) forever. + try { + return await deps.oauth.login(serverId); + } finally { + // Success and failure both may have moved the connection state + // (needs-auth → connected, or a fresh needs-auth after a refused + // consent screen) — the renderer needs whichever it is. + changed(deps); + } + }); + deps.ipcMain.handle('mcp:cancelLogin', async (_event, serverId: string) => { + const cancelled = deps.oauth.cancelLogin(serverId); + // The round's own rejection path abandons the persisted pending state; + // the renderer just needs the resulting statuses. + if (cancelled) changed(deps); + return cancelled; + }); + deps.ipcMain.handle('mcp:logout', async (_event, serverId: string) => { + // Like mcp:login, no preflight here: readiness runs INSIDE the + // controller under its round deadline, so a stalled store cannot park + // the renderer's logout lock forever. + try { + return await deps.oauth.logout(serverId); + } finally { + changed(deps); + } + }); +} + +/** Servers whose stored credentials this commit orphans: removed outright, + * repointed to a different endpoint, or converted away from remote. An + * unchanged endpoint keeps its credentials. Removals retire regardless of + * kind — a stale record under a formerly-remote id must not survive the id + * being freed for reuse. */ +function credentialRetirements(current: McpConfigFile, next: McpConfigFile): string[] { + const retired: string[] = []; + for (const [serverId, server] of Object.entries(current.mcpServers)) { + const incoming = Object.hasOwn(next.mcpServers, serverId) + ? next.mcpServers[serverId] + : undefined; + if (!incoming) { + retired.push(serverId); + continue; + } + if (isMcpStdioConfig(server)) continue; + if (isMcpStdioConfig(incoming) || incoming.url !== server.url) retired.push(serverId); + } + return retired; } function changed(deps: McpIpcMainDeps): void { diff --git a/apps/desktop/src/main/mcp-oauth-controller.ts b/apps/desktop/src/main/mcp-oauth-controller.ts new file mode 100644 index 0000000000..694d90d7ef --- /dev/null +++ b/apps/desktop/src/main/mcp-oauth-controller.ts @@ -0,0 +1,502 @@ +// apps/desktop/src/main/mcp-oauth-controller.ts +// +// Interactive OAuth for remote MCP servers: the RFC 8252 native-app shape. +// login() binds a loopback callback listener, asks the manager for the +// authorization URL, opens it in the system browser (never an embedded +// webview — the user must be able to see the address bar), waits for the +// redirect, and hands the code back to the manager for the token exchange. +// +// The listener binds 127.0.0.1 on an ephemeral port by default. A server +// whose OAuth client was registered statically pins `oauth.callbackPort` +// in its config, because its registered redirect URI carries a fixed port. + +import { randomBytes } from 'node:crypto'; +import { createServer, type Server, type ServerResponse } from 'node:http'; +import { isLoopbackHost, type McpServerStatus } from '@maka/core/mcp'; +import type { McpAuthorizationStart } from '@maka/mcp'; + +const CALLBACK_PATH = '/callback'; +const DEFAULT_LOGIN_TIMEOUT_MS = 5 * 60_000; +/** The terminal-cleanup wait is guarding against a wedged credential lane, + * not doing real work — a few seconds is enough; a round that already + * timed out must not hold its caller for a second full round. */ +const ABANDON_GRACE_MS = 5_000; + +export interface McpOAuthLoginManager { + startAuthorization( + serverId: string, + redirectUrl: string, + options?: { state?: string; signal?: AbortSignal }, + ): Promise; + finishAuthorization( + serverId: string, + callback: { code: string; iss?: string; state?: string }, + options?: { signal?: AbortSignal }, + ): Promise; + clearAuthorization( + serverId: string, + options?: { signal?: AbortSignal }, + ): Promise; + /** Clears a persisted-but-dead pending round (verifier/redirect/state), + * keeping tokens and client registration intact. */ + abandonAuthorization(serverId: string): Promise; + pendingAuthorization( + serverId: string, + ): Promise<{ redirectUrl: string; state?: string } | undefined>; + status(serverId: string): McpServerStatus | undefined; +} + +export interface McpOAuthControllerDeps { + manager: McpOAuthLoginManager; + openExternal(url: string): Promise; + /** Awaited (under the round deadline) before login and before a resumed + * round's token exchange: the listener rebinds from storage alone (early, + * independent of connects), but the manager needs the server config. */ + ensureReady?(): Promise; + /** Resolves the configured static callback port for a server. Owned by + * the controller so the store read rides the SAME round deadline — an + * IPC-side preflight await would sit outside it and park the renderer's + * login lock forever if the store hung. */ + callbackPort?(serverId: string): Promise; + /** The IPC layer's config-mutation lane (createMcpExclusiveLane). The + * login/resume CLAIM travels through it, so a claim can never land + * between a config transaction's active-login check and its write — and + * a claim never lands while a transaction is mid-flight. The round itself + * runs outside the lane; only the claim is serialized. */ + claimLane?: (work: () => Promise) => Promise; + loginTimeoutMs?: number; + copy?: { successTitle: string; successBody: string; failureTitle: string }; +} + +export interface McpOAuthController { + login(serverId: string): Promise; + logout(serverId: string): Promise; + /** Whether a login round currently owns this server. The IPC layer + * refuses config mutation for a server mid-round: renderer-side locks are + * advisory, and a config change under a live browser round would race the + * callback against a changed or absent server. */ + isActive(serverId: string): boolean; + /** Ends an in-flight round the way a timeout would: the race rejects, + * the signal fences the round's late writes, the guard releases, and the + * terminal cleanup clears the persisted pending state — so the ordinary + * "clicked Login, closed the tab" path is not a five-minute trap in + * which every config edit for the server is vetoed. Returns false when + * no round is active. */ + cancelLogin(serverId: string): boolean; + /** Rebinds the loopback listener for a login round the manager persisted + * before an app restart, so the browser's callback still lands. Resolves + * undefined when there is nothing to resume (or the port is taken). */ + resumeLogin(serverId: string): Promise; +} + +export function createMcpOAuthController(deps: McpOAuthControllerDeps): McpOAuthController { + const active = new Set(); + const roundAborts = new Map void>(); + const timeoutMs = deps.loginTimeoutMs ?? DEFAULT_LOGIN_TIMEOUT_MS; + const claimLane = deps.claimLane ?? (async (work: () => Promise) => work()); + /** Claims the per-server round guard through the shared config-mutation + * lane (when wired): after this resolves, every config transaction sees + * `isActive()` true, and no transaction was mid-flight when it landed. */ + const claim = (serverId: string): Promise => + claimLane(async () => { + if (active.has(serverId)) return false; + active.add(serverId); + return true; + }); + /** Terminal-failure cleanup is BOUNDED: the abandon frequently shares the + * exact resource that stalled the round (a wedged credential lane), and + * awaiting it unbounded would park the rejection — and the renderer's + * lock — forever. A late abandon completing afterwards is harmless: it is + * version-pinned against newer rounds. */ + const abandonGraceMs = Math.min(timeoutMs, ABANDON_GRACE_MS); + const boundedAbandon = (serverId: string): Promise => + new Promise((resolve) => { + const timer = setTimeout(resolve, abandonGraceMs); + timer.unref?.(); + void deps.manager + .abandonAuthorization(serverId) + .catch(() => {}) + .finally(() => { + clearTimeout(timer); + resolve(); + }); + }); + const copy = deps.copy ?? { + successTitle: 'Login complete', + successBody: 'You can close this tab and return to Maka.', + failureTitle: 'Login failed', + }; + + async function login(serverId: string): Promise { + if (!(await claim(serverId))) { + throw new Error(`MCP login already in progress: ${serverId}`); + } + let deadlineRef: { abort(reason: Error): void } | undefined; + roundAborts.set(serverId, (reason) => deadlineRef?.abort(reason)); + // One deadline for the whole round — discovery, browser wait, and token + // exchange. A metadata or token endpoint that accepts the connection and + // never answers must not hold the in-progress guard and the loopback + // listener forever. + const deadline = createLoginDeadline(timeoutMs); + deadlineRef = deadline; + try { + // Preflight rides the same deadline: a hung readiness gate or a + // wedged credential/config store must release the round like any + // other stalled stage, not hold the guard forever. + await deadline.race(Promise.resolve(deps.ensureReady?.())); + const callbackPort = deps.callbackPort + ? await deadline.race(deps.callbackPort(serverId)) + : undefined; + const state = randomBytes(16).toString('hex'); + const callback = await startCallbackListener({ + port: callbackPort, + state, + copy, + }); + try { + const start = await deadline.race( + deps.manager.startAuthorization(serverId, callback.redirectUrl, { + state, + signal: deadline.signal, + }), + ); + if (start.status === 'authorized') { + // Stored or refreshed credentials already satisfied the server — + // no browser round needed. + return requireStatus(deps.manager, serverId); + } + // Deliberate defence-in-depth: McpClientManager already refused + // anything this check would (its assertTransportSecurity is + // strictly stronger), so against the real manager this cannot fire. + // It stays for any future McpOAuthLoginManager implementer — the + // controller hands this URL to shell.openExternal and must not + // trust the interface contract alone with file:, javascript: or a + // custom app protocol. + // A cleartext authorization URL off the machine would also hand the + // whole login (and the code coming back) to the network, so http is + // loopback-only — the same rule the config store applies to + // endpoint URLs. + const authorizationUrl = new URL(start.authorizationUrl); + const isSecure = + authorizationUrl.protocol === 'https:' || + (authorizationUrl.protocol === 'http:' && isLoopbackHost(authorizationUrl.hostname)); + if (!isSecure) { + throw new Error( + `Authorization URL for MCP server "${serverId}" refused: non-loopback URLs require https`, + ); + } + // TODO(disclosure): before this opens, the confirm step in the UI + // (#2921) should show `start.issuer` and `start.scopes` — the host, + // path, scope and resource of this URL are all chosen by the + // untrusted MCP server, and the system browser's address bar is + // currently the only disclosure the user gets. + // The shell launch rides the same deadline: a hung `openExternal` + // must not hold the listener and the active guard past it. + await deadline.race(deps.openExternal(authorizationUrl.toString())); + const payload = await deadline.race(callback.authorizationCode); + return await deadline.race( + deps.manager.finishAuthorization( + serverId, + { ...payload, state }, + { signal: deadline.signal }, + ), + ); + } finally { + // Timeout included: the port is released on every exit. + callback.close(); + } + } catch (error) { + // The round is terminally dead — denied, timed out, or the browser + // never opened. Its persisted verifier/redirect/state must go with + // it: otherwise the boot resume rebinds a listener for a round that + // can never complete and occupies the login guard on every restart. + await boundedAbandon(serverId); + throw error; + } finally { + deadline.cancel(); + roundAborts.delete(serverId); + active.delete(serverId); + } + } + + async function resumeLogin(serverId: string): Promise { + // Claim the guard before the first await: checking, awaiting, and only + // then claiming would let a concurrent login() start a second round — + // and either round's cleanup would release the other's guard. + if (!(await claim(serverId))) return undefined; + const deadline = createLoginDeadline(timeoutMs); + roundAborts.set(serverId, (reason) => deadline.abort(reason)); + try { + // The pending lookup reads the credential store, which can wait on a + // contended file lock — the guard must not outlive the deadline here + // either. + const pending = await deadline.race(deps.manager.pendingAuthorization(serverId)); + // Without the persisted state the callback cannot be verified; without + // a fixed port the browser's redirect target is gone. Either way the + // round is unresumable — the user simply logs in again. + if (!pending?.state) return undefined; + const redirectUrl = new URL(pending.redirectUrl); + const port = Number(redirectUrl.port); + if (redirectUrl.hostname !== '127.0.0.1' || !Number.isInteger(port) || port === 0) { + return undefined; + } + let callback: Awaited>; + try { + callback = await startCallbackListener({ + port, + state: pending.state, + copy, + }); + } catch { + // The recorded port is taken (or cannot be bound) — the round is + // unresumable, which the contract reports as undefined, not as a + // failure: the user simply logs in again. + return undefined; + } + try { + const payload = await deadline.race(callback.authorizationCode); + await deadline.race(Promise.resolve(deps.ensureReady?.())); + return await deadline.race( + deps.manager.finishAuthorization( + serverId, + { ...payload, state: pending.state }, + { signal: deadline.signal }, + ), + ); + } catch (error) { + // Same terminality as login(): a resumed round that denied or + // timed out is dead — clear it rather than resume it again on the + // next restart. + await boundedAbandon(serverId); + throw error; + } finally { + callback.close(); + } + } finally { + deadline.cancel(); + roundAborts.delete(serverId); + active.delete(serverId); + } + } + + return { + login, + async logout(serverId) { + // Same bounded-round rule as login: readiness and the credential + // clear run under one deadline, so a stalled store or a hung + // reconnect cannot park the renderer's logout lock forever. + const deadline = createLoginDeadline(timeoutMs); + try { + await deadline.race(Promise.resolve(deps.ensureReady?.())); + // The signal travels INTO the erase: racing alone would abandon the + // caller while the stalled clear kept running and could tombstone + // the fresh tokens a NEWER login stores in the meantime. + return await deadline.race( + deps.manager.clearAuthorization(serverId, { signal: deadline.signal }), + ); + } finally { + deadline.cancel(); + } + }, + resumeLogin, + cancelLogin(serverId) { + const abort = roundAborts.get(serverId); + if (!abort) return false; + abort(new Error('Login cancelled')); + return true; + }, + isActive: (serverId) => active.has(serverId), + }; +} + +/** What the loopback listener hands back after verifying the state: the + * full protocol payload the SDK still needs to validate — the code AND the + * RFC 9207 `iss` parameter. Truncating to a bare code here would silently + * disable the SDK's authorization-server mix-up defense. */ +export interface McpAuthorizationCallbackPayload { + code: string; + iss?: string; +} + +interface CallbackListener { + redirectUrl: string; + authorizationCode: Promise; + close(): void; +} + +/** One deadline covering a complete login round. Callers race every stage + * against it — discovery/probe, the browser wait, and the token exchange — + * so a hung remote endpoint cannot park the round past the timeout. The + * deadline's `signal` travels INTO the round: racing alone would only + * abandon the caller while the underlying OAuth flow kept running and could + * write verifier/state/tokens after a second login started — the signal + * aborts its requests and fences its late storage writes. */ +function createLoginDeadline(timeoutMs: number): { + race(operation: Promise): Promise; + signal: AbortSignal; + cancel(): void; + abort(reason: Error): void; +} { + const round = new AbortController(); + let cancel!: () => void; + let abort!: (reason: Error) => void; + const expired = new Promise((_, reject) => { + const timer = setTimeout(() => { + round.abort(new Error('Timed out waiting for the browser login')); + reject(new Error('Timed out waiting for the browser login')); + }, timeoutMs); + timer.unref(); + cancel = () => clearTimeout(timer); + // A user cancellation ends the round the same way a timeout does: the + // race rejects and the signal fences the round's late writes. + abort = (reason: Error) => { + clearTimeout(timer); + round.abort(reason); + reject(reason); + }; + }); + // Nothing may be racing when the deadline fires (or after cancel) — the + // rejection must not crash the process as unhandled. + expired.catch(() => {}); + return { + race: (operation: Promise) => Promise.race([operation, expired]), + signal: round.signal, + cancel, + abort, + }; +} + +function startCallbackListener(input: { + port?: number; + state: string; + copy: { successTitle: string; successBody: string; failureTitle: string }; +}): Promise { + return new Promise((resolveListener, rejectListener) => { + let settleCode!: (payload: McpAuthorizationCallbackPayload) => void; + let failCode!: (error: Error) => void; + const authorizationCode = new Promise((resolve, reject) => { + settleCode = resolve; + failCode = reject; + }); + // The 'authorized' short-circuit never awaits this promise, and close() + // rejects it — mark it handled so that path can't crash the process. + authorizationCode.catch(() => {}); + + let expectedHost: string | undefined; + const server: Server = createServer((request, response) => { + // Same rules as this repo's other loopback listener (cdp-bridge): + // the Host must be the loopback authority this listener bound — a + // DNS-rebinding page cannot present it — and a request carrying a + // browser Origin is a cross-origin fetch, not the redirect + // navigation this endpoint exists for. + if (expectedHost !== undefined && request.headers.host !== expectedHost) { + response.writeHead(403).end(); + return; + } + if (request.headers.origin !== undefined) { + response.writeHead(403).end(); + return; + } + const url = new URL(request.url ?? '/', 'http://127.0.0.1'); + if (url.pathname !== CALLBACK_PATH) { + response.writeHead(404).end(); + return; + } + // State first: only a callback that proves it belongs to this login + // round may affect it. Handling `error` before the state check would + // let anyone on loopback abort a real login with a forged + // access_denied. + const returnedState = url.searchParams.get('state'); + if (returnedState !== input.state) { + respond(response, 400, input.copy.failureTitle, 'Invalid callback.'); + return; + } + const error = url.searchParams.get('error'); + if (error) { + // Fixed local copy only: `error_description` is the authorization + // server's arbitrary prose, and rendering it on a page the user + // reads as Maka's is a phishing surface even HTML-escaped. The + // sanitized code is the one server-controlled token shown. + respond(response, 200, input.copy.failureTitle, sanitizeOAuthErrorCode(error)); + failCode(new Error(`Authorization failed: ${sanitizeOAuthErrorCode(error)}`)); + return; + } + const code = url.searchParams.get('code'); + if (!code) { + respond(response, 400, input.copy.failureTitle, 'Invalid callback.'); + return; + } + respond(response, 200, input.copy.successTitle, input.copy.successBody); + const iss = url.searchParams.get('iss'); + settleCode({ code, ...(iss !== null ? { iss } : {}) }); + }); + server.on('error', (error) => { + rejectListener(error); + }); + server.listen(input.port ?? 0, '127.0.0.1', () => { + const address = server.address(); + if (address === null || typeof address === 'string') { + rejectListener(new Error('Callback listener has no address')); + return; + } + expectedHost = `127.0.0.1:${address.port}`; + resolveListener({ + redirectUrl: `http://127.0.0.1:${address.port}${CALLBACK_PATH}`, + authorizationCode, + close: () => { + failCode(new Error('Login cancelled')); + server.close(); + // Callback responses have flushed by the time close() runs in the + // login flow; lingering keep-alive sockets must not hold the port. + server.closeAllConnections(); + }, + }); + }); + }); +} + +/** The registered OAuth error codes this flow can encounter (RFC 6749 §4.1.2.1 + * and §5.2, plus the OIDC interaction codes). A strict allowlist, not a shape + * check: the parameter is attacker-writable, and anything that merely LOOKS + * like a code (`opaqueSecret123`) must not tunnel through to the renderer. */ +const OAUTH_ERROR_CODES = new Set([ + 'invalid_request', + 'unauthorized_client', + 'access_denied', + 'unsupported_response_type', + 'invalid_scope', + 'server_error', + 'temporarily_unavailable', + 'invalid_client', + 'invalid_grant', + 'unsupported_grant_type', + 'interaction_required', + 'login_required', + 'consent_required', +]); + +function sanitizeOAuthErrorCode(value: string): string { + return OAUTH_ERROR_CODES.has(value) ? value : 'unknown_error'; +} + +function requireStatus(manager: McpOAuthLoginManager, serverId: string): McpServerStatus { + const status = manager.status(serverId); + if (!status) throw new Error(`Unknown MCP server: ${serverId}`); + return status; +} + +function respond(response: ServerResponse, statusCode: number, title: string, body: string): void { + const html = `${escapeHtml(title)}

${escapeHtml(title)}

${escapeHtml(body)}

`; + response + .writeHead(statusCode, { + 'content-type': 'text/html; charset=utf-8', + // The redirect URL carried the one-time code; the response must not + // let the round-tripped page (and its URL) sit in a shared cache. + 'cache-control': 'no-store', + }) + .end(html); +} + +function escapeHtml(value: string): string { + return value.replace(/[&<>"']/gu, (char) => `&#${char.charCodeAt(0)};`); +} diff --git a/apps/desktop/src/main/runtime-host-boot.ts b/apps/desktop/src/main/runtime-host-boot.ts index 6171c61c71..9cfac2aa48 100644 --- a/apps/desktop/src/main/runtime-host-boot.ts +++ b/apps/desktop/src/main/runtime-host-boot.ts @@ -22,12 +22,15 @@ import { loadOrCreateRuntimeHostClientInstanceId, } from "@maka/runtime-host/client"; import type { WorkspaceTarget } from "@maka/runtime-host/protocol"; -import { McpClientManager } from "@maka/mcp"; +import { createCredentialMcpOAuthStorage, McpClientManager } from "@maka/mcp"; import { createSettingsStore, createMcpConfigStore, + createFileCredentialStore, } from "@maka/storage"; import { resolveStorageRoot } from "@maka/storage/root-authority"; + +import { createMcpOAuthController } from "./mcp-oauth-controller.js"; import { registerAppClientIpc, registerAppIpc } from "./app-ipc-main.js"; import { createAppQuitCoordinator } from "./app-quit-coordinator.js"; import { createAppUpdateService } from "./app-update-service.js"; @@ -60,7 +63,7 @@ import { mainProcessLogBuffer } from "./main-process-diagnostics.js"; import { resolveDesktopSessionWorkspace, } from "./new-session-project.js"; -import { registerMcpIpcMain } from "./mcp-ipc-main.js"; +import { createMcpExclusiveLane, registerMcpIpcMain } from "./mcp-ipc-main.js"; import { createOnboardingService } from "./onboarding-service.js"; import { registerOnboardingIpc } from "./onboarding-ipc-main.js"; import { @@ -208,6 +211,23 @@ const mcpConfigStore = createMcpConfigStore(workspaceRoot); const mcpManager = new McpClientManager({ clientName: "maka-desktop", clientVersion: app.getVersion(), + oauthStorage: createCredentialMcpOAuthStorage( + createFileCredentialStore(workspaceRoot), + ), +}); +// One lane shared by config transactions and login claims: "no login is +// active" checked inside a transaction cannot be invalidated by a claim +// landing between the check and the write. +const mcpExclusiveLane = createMcpExclusiveLane(); +const mcpOAuthController = createMcpOAuthController({ + manager: mcpManager, + claimLane: mcpExclusiveLane, + openExternal: (url) => shell.openExternal(url), + ensureReady: () => ensureMcpReady(), + callbackPort: async (serverId) => { + const server = (await mcpConfigStore.get()).mcpServers[serverId]; + return server && "url" in server ? server.oauth?.callbackPort : undefined; + }, }); let mcpStartup: Promise | undefined; function ensureMcpReady(): Promise { @@ -723,6 +743,33 @@ updateService.start(); void ensureMcpReady() .then(() => mcpCapabilityPublisher.refreshIfChanged()) .catch((error) => console.error("[runtime-host] MCP startup failed:", error)); +// A login round persists its verifier and callback port; if the app +// restarted mid-round, rebind the listener so the browser's redirect still +// lands instead of hitting a dead port. Deliberately NOT chained behind the +// connect/publish sequence above: a slow server or a publish failure must +// not delay or block the rebind — it needs only the persisted state, and +// the controller awaits readiness itself before the token exchange. +void mcpConfigStore + .get() + .then((config) => { + for (const serverId of Object.keys(config.mcpServers)) { + void mcpOAuthController + .resumeLogin(serverId) + // No explicit mcp:changed here: a successful resume ends in + // finishAuthorization → reconnect, whose onChange handler already + // emits AND refreshes capabilities — a second identical emit here + // was strictly weaker. + .catch((error) => + console.error( + `[runtime-host] MCP login resume failed for ${serverId}:`, + error, + ), + ); + } + }) + .catch((error) => + console.error("[runtime-host] MCP login resume scan failed:", error), + ); void clientSettingsEffects .refresh(false) @@ -837,6 +884,8 @@ function registerHostClientIpc( ipcMain: scopedIpc, store: mcpConfigStore, manager: mcpManager, + oauth: mcpOAuthController, + exclusiveLane: mcpExclusiveLane, ensureReady: ensureMcpReady, publishCapabilities: mcpCapabilityPublisher.refreshIfChanged, onPublicationError: (error) => diff --git a/apps/desktop/src/preload/bridge-contract.d.ts b/apps/desktop/src/preload/bridge-contract.d.ts index 3e0ff10b8c..c0fd432e4e 100644 --- a/apps/desktop/src/preload/bridge-contract.d.ts +++ b/apps/desktop/src/preload/bridge-contract.d.ts @@ -104,6 +104,7 @@ import type { import type { Result } from '@maka/core/result'; import type { CreateSessionRequestInput } from '@maka/core/runtime-inputs'; import type { + McpConfigAddResult, McpConfigFile, McpServerConfig, McpServerStatus, @@ -744,11 +745,18 @@ export interface MakaBridge { getConfig(): Promise; listStatuses(): Promise; setConfig(config: McpConfigFile): Promise; + /** Adds a new server; a taken id comes back as `{ status: 'exists' }` + * instead of an error, so the dialog can put it on the id field. */ + add(serverId: string, config: McpServerConfig): Promise; upsert(serverId: string, config: McpServerConfig): Promise; install(serverId: string, config: McpServerConfig): Promise; remove(serverId: string): Promise; cancelInstall(serverId: string): Promise; test(serverId: string): Promise; + login(serverId: string): Promise; + /** Ends an in-flight login round; resolves false when none is active. */ + cancelLogin(serverId: string): Promise; + logout(serverId: string): Promise; subscribeChanges(handler: (statuses: McpServerStatus[]) => void): () => void; }; settings: { diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index f17b8ea656..41878d3f91 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -150,6 +150,7 @@ import { import type { Result } from '@maka/core/result'; import type { CreateSessionRequestInput } from '@maka/core/runtime-inputs'; import type { + McpConfigAddResult, McpConfigFile, McpServerConfig, McpServerStatus, @@ -1978,6 +1979,9 @@ const makaBridge = { setConfig(config: McpConfigFile): Promise { return invokeActiveRuntimeHost('mcp:setConfig', config); }, + add(serverId: string, config: McpServerConfig): Promise { + return invokeActiveRuntimeHost('mcp:add', serverId, config); + }, upsert(serverId: string, config: McpServerConfig): Promise { return invokeActiveRuntimeHost('mcp:upsert', serverId, config); }, @@ -1993,6 +1997,19 @@ const makaBridge = { test(serverId: string): Promise { return invokeActiveRuntimeHost('mcp:test', serverId); }, + // Same scoped seam as every other MCP method: the handlers live on the + // Runtime Host's ScopedIpcMain, whose first argument is the host ref — + // a raw invoke would put serverId in that slot and fail the scope check + // before the handler ever ran. + login(serverId: string): Promise { + return invokeActiveRuntimeHost('mcp:login', serverId); + }, + cancelLogin(serverId: string): Promise { + return invokeActiveRuntimeHost('mcp:cancelLogin', serverId); + }, + logout(serverId: string): Promise { + return invokeActiveRuntimeHost('mcp:logout', serverId); + }, subscribeChanges(handler: (statuses: McpServerStatus[]) => void): () => void { return subscribeActiveRuntimeHostEvent('mcp:changed', handler); }, diff --git a/apps/desktop/src/renderer/locales/mcp-copy.ts b/apps/desktop/src/renderer/locales/mcp-copy.ts index dc4d006ba8..c81a5fdeee 100644 --- a/apps/desktop/src/renderer/locales/mcp-copy.ts +++ b/apps/desktop/src/renderer/locales/mcp-copy.ts @@ -5,12 +5,14 @@ export type McpCopy = { load: string; install(name: string): string; cancelInstall(name: string): string; save: string; import: string; update: string; test: string; remove: string; unavailableStatus: string; mapLine(line: number): string; importObject: string; importVersion(version: string): string; importServersObject: string; importProtocolVersion: string; + login: string; logout: string; serverBusy: string; }; toast: { templateInstalled(name: string): string; templateInstalledDetail: string; installed(name: string): string; installedDetail: string; installCancelled(name: string): string; saved: string; savedDetail: string; imported: string; importedDetail(count: number): string; connectionOk: string; toolLatency(count: number, latencyMs: number): string; connectionFailed: string; removed: string; + loginOk(id: string): string; loggedOut(id: string): string; }; remove: { title(id: string): string; description: string; confirm: string; cancel: string }; page: { @@ -27,15 +29,17 @@ export type McpCopy = { toolsLabel: string; statusLabel: string; protocolLabel: string; negotiatedProtocol(era: 'legacy' | 'modern', revision: string): string; inspectorOpened(id: string): string; + needsAuthTitle: string; needsAuthBody: string; }; card: { macOnly: string; manage: string; cancellingAria(name: string): string; cancelAria(name: string): string; installAria(name: string): string; cancelling: string; cancel: string; install: string; }; row: { - testing: string; test: string; edit: string; + test: string; edit: string; delete: string; tools(count: number): string; - disabled: string; disconnected: string; connecting: string; connected(count: number): string; failed: string; + disabled: string; disconnected: string; connecting: string; connected: string; failed: string; + needsAuth: string; login: string; cancelLogin: string; logout: string; }; editor: { importTitle: string; editTitle(id: string): string; addTitle: string; importSubtitle: string; manualSubtitle: string; @@ -43,8 +47,9 @@ export type McpCopy = { importConnect: string; transportAria: string; localStdio: string; remoteUrl: string; serverId: string; command: string; commandPlaceholder: string; commandHelp: string; workingDirectory: string; workingDirectoryPlaceholder: string; environment: string; environmentHelp: string; - url: string; headers: string; headersHelp: string; saveConnect: string; - required: string; invalidUrl: string; unbalancedQuote: string; + url: string; urlCredentials: string; headers: string; headersHelp: string; + headersOAuthHelp: string; oauthAuthorizationConflict: string; saveConnect: string; + required: string; invalidUrl: string; insecureUrl: string; unbalancedQuote: string; duplicateId: string; advanced: string; transportLabel: string; transportAuto: string; transportStreamableHttp: string; transportLegacySse: string; protocolLabel: string; protocolLegacy: string; protocolAuto: string; protocolModern: string; protocolHelp: string; sseProtocolHelp: string; @@ -59,6 +64,8 @@ const MCP_COPY = { mapLine: (line) => `第 ${line} 行应为 KEY=value`, importObject: 'MCP JSON 必须是 object', importVersion: (version) => `不支持 MCP 配置版本 ${version}，当前支持 version 1 和 2`, importServersObject: 'mcpServers 必须是 object', importProtocolVersion: '含 protocol 的 MCP 配置必须显式使用 version 2', + login: 'MCP 登录失败', logout: 'MCP 退出登录失败', + serverBusy: '该 server 正在执行其他操作（例如登录），请等它完成后再保存。', }, toast: { templateInstalled: (name) => `${name} 模板已安装`, templateInstalledDetail: '请在「已安装」中完成凭据配置，再启用连接。', @@ -66,6 +73,7 @@ const MCP_COPY = { saved: 'MCP 已保存', savedDetail: '新工具会从下一次 agent turn 开始生效。', imported: '已导入 MCP', importedDetail: (count) => `本次导入 ${count} 个 server。`, connectionOk: 'MCP 连接正常', toolLatency: (count, latencyMs) => `${count} 个工具 · ${latencyMs} ms`, connectionFailed: 'MCP 连接失败', removed: 'MCP 已删除', + loginOk: (id) => `${id} 已完成登录`, loggedOut: (id) => `已退出 ${id} 的登录`, }, remove: { title: (id) => `删除 MCP「${id}」？`, description: '它提供的工具会从下一次 agent turn 中移除，配置无法自动恢复。', confirm: '删除', cancel: '取消' }, page: { @@ -83,15 +91,17 @@ const MCP_COPY = { toolsLabel: '工具', statusLabel: '状态', protocolLabel: 'MCP 协议', negotiatedProtocol: (era, revision) => `${era === 'modern' ? '现代' : '传统'} · ${revision}`, inspectorOpened: (id) => `已打开 ${id} 的详情`, + needsAuthTitle: '需要登录', needsAuthBody: '该服务器要求浏览器授权。点击「登录」会打开系统浏览器完成授权，凭据保存在本机。', }, card: { macOnly: '仅 macOS', manage: '管理', cancellingAria: (name) => `正在取消安装 ${name}`, cancelAria: (name) => `取消安装 ${name}`, installAria: (name) => `安装 ${name}`, cancelling: '正在取消…', cancel: '取消安装', install: '安装', }, row: { - testing: '测试中…', test: '测试', edit: '编辑', + test: '测试', edit: '编辑', delete: '删除', tools: (count) => `${count} 个工具`, - disabled: '已停用', disconnected: '未连接', connecting: '连接中', connected: (count) => `${count} 个工具`, failed: '连接失败', + disabled: '已停用', disconnected: '未连接', connecting: '连接中', connected: '已连接', failed: '连接失败', + needsAuth: '需要登录', login: '登录', cancelLogin: '取消登录', logout: '退出登录', }, editor: { importTitle: '通过 JSON 导入', editTitle: (id) => `编辑 ${id}`, addTitle: '添加 MCP', importSubtitle: '粘贴 mcpServers 配置，同名 server 会被更新。', @@ -102,9 +112,13 @@ const MCP_COPY = { commandPlaceholder: 'npx -y @modelcontextprotocol/server-filesystem /path/to/folder', commandHelp: '完整命令行；含空格的参数用引号包裹，不经过 shell 解析。', workingDirectory: '工作目录', workingDirectoryPlaceholder: '可选，例如 /path/to/project', - environment: '环境变量', environmentHelp: '每行一个 KEY=value；按 MCP 要求填写。', url: 'MCP URL', headers: 'HTTP 请求头', headersHelp: '每行一个 Header=value。', + environment: '环境变量', environmentHelp: '每行一个 KEY=value；按 MCP 要求填写。', url: 'MCP URL', + urlCredentials: 'URL 不能包含内嵌凭据（user:pass@），请改用请求头配置。', + headers: 'HTTP 请求头', headersHelp: '每行一个 Header=value。', + headersOAuthHelp: '每行一个 Header=value。该服务器使用 OAuth 登录，Authorization 由登录管理，不能在此配置。', + oauthAuthorizationConflict: '该服务器使用 OAuth 登录：Authorization 请求头由登录流程管理，请移除此行。', saveConnect: '保存并连接', - required: '此字段为必填项。', invalidUrl: '请输入有效的 HTTP 或 HTTPS URL。', unbalancedQuote: '引号未闭合。', + required: '此字段为必填项。', invalidUrl: '请输入有效的 HTTP 或 HTTPS URL。', insecureUrl: '非本机地址需使用 HTTPS。', unbalancedQuote: '引号未闭合。', duplicateId: '已存在同名服务器；换一个 ID，或编辑现有配置。', advanced: '高级设置', transportLabel: '传输协议', transportAuto: '自动回退', transportStreamableHttp: 'Streamable HTTP', transportLegacySse: '旧版 SSE', protocolLabel: '协议偏好', protocolLegacy: '传统', protocolAuto: '自动协商', protocolModern: '仅 2026-07-28', protocolHelp: '旧配置默认使用传统协议；自动协商会根据 server 能力选择协议。', sseProtocolHelp: '旧版 SSE 仅支持传统协议。', @@ -117,6 +131,8 @@ const MCP_COPY = { mapLine: (line) => `Line ${line} must use KEY=value`, importObject: 'MCP JSON must be an object', importVersion: (version) => `Unsupported MCP config version ${version}; versions 1 and 2 are supported`, importServersObject: 'mcpServers must be an object', importProtocolVersion: 'MCP configurations containing protocol must explicitly use version 2', + login: 'MCP login failed', logout: 'MCP logout failed', + serverBusy: 'Another operation (such as a login) owns this server — wait for it to finish before saving.', }, toast: { templateInstalled: (name) => `${name} template installed`, templateInstalledDetail: 'Finish configuring credentials under Installed before enabling the connection.', @@ -124,6 +140,7 @@ const MCP_COPY = { saved: 'MCP saved', savedDetail: 'New tools take effect from the next agent turn.', imported: 'MCP imported', importedDetail: (count) => `Imported ${count} ${count === 1 ? 'server' : 'servers'}.`, connectionOk: 'MCP connection healthy', toolLatency: (count, latencyMs) => `${count} ${count === 1 ? 'tool' : 'tools'} · ${latencyMs} ms`, connectionFailed: 'MCP connection failed', removed: 'MCP deleted', + loginOk: (id) => `${id} login complete`, loggedOut: (id) => `Logged out of ${id}`, }, remove: { title: (id) => `Delete MCP “${id}”?`, description: 'Its tools will be removed from the next agent turn, and the configuration cannot be restored automatically.', confirm: 'Delete', cancel: 'Cancel' }, page: { @@ -141,15 +158,17 @@ const MCP_COPY = { toolsLabel: 'Tools', statusLabel: 'Status', protocolLabel: 'MCP protocol', negotiatedProtocol: (era, revision) => `${era === 'modern' ? 'Modern' : 'Legacy'} · ${revision}`, inspectorOpened: (id) => `${id} details opened`, + needsAuthTitle: 'Login required', needsAuthBody: 'This server requires browser authorization. Log in opens your system browser; credentials are stored on this machine.', }, card: { macOnly: 'macOS only', manage: 'Manage', cancellingAria: (name) => `Cancelling installation of ${name}`, cancelAria: (name) => `Cancel installation of ${name}`, installAria: (name) => `Install ${name}`, cancelling: 'Cancelling…', cancel: 'Cancel installation', install: 'Install', }, row: { - testing: 'Testing…', test: 'Test', edit: 'Edit', + test: 'Test', edit: 'Edit', delete: 'Delete', tools: (count) => `${count} ${count === 1 ? 'tool' : 'tools'}`, - disabled: 'Disabled', disconnected: 'Disconnected', connecting: 'Connecting', connected: (count) => `${count} ${count === 1 ? 'tool' : 'tools'}`, failed: 'Connection failed', + disabled: 'Disabled', disconnected: 'Disconnected', connecting: 'Connecting', connected: 'Connected', failed: 'Connection failed', + needsAuth: 'Login required', login: 'Log in', cancelLogin: 'Cancel login', logout: 'Log out', }, editor: { importTitle: 'Import from JSON', editTitle: (id) => `Edit ${id}`, addTitle: 'Add MCP', importSubtitle: 'Paste an mcpServers configuration; servers with matching names will be updated.', @@ -160,9 +179,12 @@ const MCP_COPY = { commandPlaceholder: 'npx -y @modelcontextprotocol/server-filesystem /path/to/folder', commandHelp: 'Full command line; quote arguments containing spaces. Not interpreted by a shell.', workingDirectory: 'Working directory', workingDirectoryPlaceholder: 'Optional, for example /path/to/project', - environment: 'Environment', environmentHelp: 'One KEY=value entry per line; complete the variables required by this MCP.', url: 'MCP URL', headers: 'HTTP headers', headersHelp: 'One Header=value entry per line.', + environment: 'Environment', environmentHelp: 'One KEY=value entry per line; complete the variables required by this MCP.', url: 'MCP URL', + urlCredentials: 'The URL must not embed credentials (user:pass@); configure headers instead.', headers: 'HTTP headers', headersHelp: 'One Header=value entry per line.', + headersOAuthHelp: 'One Header=value entry per line. This server signs in with OAuth; Authorization is managed by the login and cannot be configured here.', + oauthAuthorizationConflict: 'This server signs in with OAuth: the Authorization header is managed by the login flow — remove this line.', saveConnect: 'Save and connect', - required: 'This field is required.', invalidUrl: 'Enter a valid HTTP or HTTPS URL.', unbalancedQuote: 'Unclosed quote.', + required: 'This field is required.', invalidUrl: 'Enter a valid HTTP or HTTPS URL.', insecureUrl: 'Use HTTPS for non-local addresses.', unbalancedQuote: 'Unclosed quote.', duplicateId: 'A server with this ID already exists; choose another ID or edit the existing one.', advanced: 'Advanced settings', transportLabel: 'Transport', transportAuto: 'Auto fallback', transportStreamableHttp: 'Streamable HTTP', transportLegacySse: 'Legacy SSE', protocolLabel: 'Protocol preference', protocolLegacy: 'Legacy', protocolAuto: 'Auto-negotiate', protocolModern: '2026-07-28 only', protocolHelp: 'Existing configurations default to legacy; auto-negotiation selects an era from the server response.', sseProtocolHelp: 'Legacy SSE supports only the legacy protocol era.', diff --git a/apps/desktop/src/renderer/mcp-editor-validation.ts b/apps/desktop/src/renderer/mcp-editor-validation.ts index e35721986b..e005e89d1a 100644 --- a/apps/desktop/src/renderer/mcp-editor-validation.ts +++ b/apps/desktop/src/renderer/mcp-editor-validation.ts @@ -1,3 +1,4 @@ +import { isNonLoopbackCleartextHttp } from '@maka/core/mcp'; import { parseCommandLine } from './mcp-command-line.js'; export type McpEditorDraft = { @@ -5,21 +6,38 @@ export type McpEditorDraft = { kind: 'stdio' | 'remote'; commandLine: string; url: string; + headers: string; }; export type McpEditorValidationCode = | 'required' | 'invalid-url' - | 'unbalanced-quote'; + | 'insecure-url' + | 'url-credentials' + | 'unbalanced-quote' + | 'duplicate-id' + | 'oauth-authorization-conflict'; export type McpEditorErrors = Partial< - Record<'id' | 'commandLine' | 'url', McpEditorValidationCode> + Record<'id' | 'commandLine' | 'url' | 'headers', McpEditorValidationCode> >; export function validateMcpEditorDraft( draft: McpEditorDraft, + options: { + /** Server ids that would be silently overwritten by an upsert. Passed + * only in add mode — an edit legitimately writes over its own id. */ + existingIds?: readonly string[]; + /** Whether the draft carries an (invisible, opaquely round-tripped) + * oauth block. The store rejects an Authorization header alongside it; + * the dialog mirrors that rule as a field error instead of letting the + * save bounce off main as a raw untranslated toast. */ + hasOAuth?: boolean; + } = {}, ): McpEditorErrors { const errors: McpEditorErrors = {}; - if (!draft.id.trim()) errors.id = 'required'; + const id = draft.id.trim(); + if (!id) errors.id = 'required'; + else if (options.existingIds?.includes(id)) errors.id = 'duplicate-id'; if (draft.kind === 'stdio') { const parsed = parseCommandLine(draft.commandLine); @@ -40,9 +58,25 @@ export function validateMcpEditorDraft( const url = new URL(value); if (url.protocol !== 'http:' && url.protocol !== 'https:') { errors.url = 'invalid-url'; + } else if (isNonLoopbackCleartextHttp(url)) { + // The store enforces the same shared rule; validating here puts the + // error on the URL field instead of an opaque save toast. + errors.url = 'insecure-url'; + } else if (url.username || url.password) { + // Mirrors the store's embedded-credentials rejection for the same + // reason: live on the field, not a generic save-failure toast. + errors.url = 'url-credentials'; } } catch { errors.url = 'invalid-url'; } + if ( + options.hasOAuth && + draft.headers + .split(/\r?\n/u) + .some((line) => line.split('=')[0]?.trim().toLowerCase() === 'authorization') + ) { + errors.headers = 'oauth-authorization-conflict'; + } return errors; } diff --git a/apps/desktop/src/renderer/mcp-page.tsx b/apps/desktop/src/renderer/mcp-page.tsx index 60a1a73511..79f482133b 100644 --- a/apps/desktop/src/renderer/mcp-page.tsx +++ b/apps/desktop/src/renderer/mcp-page.tsx @@ -46,10 +46,9 @@ import { } from '@astryxdesign/core/Dialog'; import { Layout, LayoutContent } from '@astryxdesign/core/Layout'; import { MetadataList, MetadataListItem } from '@astryxdesign/core/MetadataList'; +import { Collapsible } from '@astryxdesign/core/Collapsible'; import { ModulePage, - RadioList, - RadioListItem, Selector, TextArea, useMountedRef, @@ -64,13 +63,11 @@ import { import { ICON_SIZE, FileCode, - Globe, Loader2, Plug, Plus, RefreshCcw, Search, - Terminal, X, } from '@maka/ui/icons'; import { getMcpCatalog, catalogEntryMatches, type McpCatalogEntry } from './mcp-catalog'; @@ -90,8 +87,10 @@ import { formatCommandLine } from './mcp-command-line'; import { validateMcpEditorDraft, type McpEditorErrors, + type McpEditorValidationCode, } from './mcp-editor-validation'; + type EditorState = | { mode: 'manual'; draft: McpEditorDraft; editingId: string | null } | { mode: 'json'; source: string } @@ -101,6 +100,31 @@ const EMPTY_CONFIG: McpConfigFile = { version: MCP_CONFIG_VERSION, mcpServers: { const MIN_INSTALL_INDICATOR_MS = 500; type InstallPhase = 'installing' | 'cancelling'; + +type McpServerOpAction = 'toggle' | 'test' | 'login' | 'logout' | 'remove' | 'save'; + +/** One mutation per server at a time — the renderer-side mirror of main's + * authoritative per-server gate. Ref-owned because claims must be + * synchronous; the onChange mirror feeds render state. */ +function createMcpServerOps(onChange?: (ops: ReadonlyMap) => void): { + claim(serverId: string, action: McpServerOpAction): boolean; + release(serverId: string): void; + actionFor(serverId: string): McpServerOpAction | undefined; +} { + const ops = new Map(); + return { + claim(serverId, action) { + if (ops.has(serverId)) return false; + ops.set(serverId, action); + onChange?.(ops); + return true; + }, + release(serverId) { + if (ops.delete(serverId)) onChange?.(ops); + }, + actionFor: (serverId) => ops.get(serverId), + }; +} type McpTab = 'market' | 'installed'; export function McpPage(props: { hubHeader?: ModuleHubHeader }) { @@ -116,6 +140,16 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { const [query, setQuery] = useState(''); const [selectedServerId, setSelectedServerId] = useState(null); const [busy, setBusy] = useState('load'); + // One mutation per server at a time: while a login round waits on the + // browser, its server's test/edit/toggle/delete stay refused instead of + // racing the callback. Ref owns the truth (claims are synchronous); + // state mirrors it for render. + const [serverOps, setServerOps] = useState>(new Map()); + const serverOpsRef = useRef( + createMcpServerOps((ops) => { + setServerOps(new Map(ops)); + }), + ); const [installPhases, setInstallPhases] = useState>({}); const cancelledInstalls = useRef(new Set()); const editorSessionRef = useRef(0); @@ -283,15 +317,43 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { async function saveDraft(event: React.FormEvent) { event.preventDefault(); if (!editor || editor.mode !== 'manual') return; - const validation = validateMcpEditorDraft(editor.draft); + const validation = validateMcpEditorDraft(editor.draft, { + existingIds: editor.editingId ? undefined : Object.keys(config.mcpServers), + hasOAuth: Boolean(editor.draft.oauth), + }); if (Object.keys(validation).length > 0) { setEditorErrors(validation); return; } setEditorErrors({}); + const serverId = editor.draft.id.trim(); + const editing = Boolean(editor.editingId); + // Every edit/save entry point — the inspector's Edit, but also + // Marketplace → Manage — routes through this claim: a save must not + // race a login round (or any other operation) that owns the server. + // Main enforces the same rule authoritatively at the IPC boundary. + if (!serverOpsRef.current.claim(serverId, 'save')) { + if (mounted.current) toast.error(copy.errors.save, copy.errors.serverBusy); + return; + } setBusy('save'); try { - const next = await window.maka.mcp.upsert(editor.draft.id.trim(), mcpConfigFromDraft(editor.draft, copy)); + // add() checks the id atomically; edit legitimately overwrites its + // own entry through upsert. + const serverConfig = mcpConfigFromDraft(editor.draft, copy); + let next: McpConfigFile; + if (editing) { + next = await window.maka.mcp.upsert(serverId, serverConfig); + } else { + const result = await window.maka.mcp.add(serverId, serverConfig); + if (result.status === 'exists') { + // The atomic guard fired between the live check and the write — + // show it on the field, not as an opaque save toast. + if (mounted.current) setEditorErrors({ id: 'duplicate-id' }); + return; + } + next = result.config; + } if (!mounted.current) return; setConfig(next); closeEditor(); @@ -300,6 +362,7 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { } catch (error) { if (mounted.current) toast.error(copy.errors.save, settingsActionErrorMessage(error, locale)); } finally { + serverOpsRef.current.release(serverId); if (mounted.current) setBusy(null); } } @@ -308,8 +371,20 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { event.preventDefault(); if (!editor || editor.mode !== 'json') return; setBusy('import'); + let claimed: string[] = []; try { const imported = parseMcpImport(editor.source, locale); + // Same claim rule as saveDraft — main's in-lane gate is the real + // authority, but claiming here means a veto (a login owning one of + // these ids) surfaces as the localized serverBusy copy instead of + // main's raw English error. + for (const serverId of Object.keys(imported.mcpServers)) { + if (!serverOpsRef.current.claim(serverId, 'save')) { + if (mounted.current) toast.error(copy.errors.import, copy.errors.serverBusy); + return; + } + claimed.push(serverId); + } const next = await window.maka.mcp.setConfig({ version: MCP_CONFIG_VERSION, mcpServers: { ...config.mcpServers, ...imported.mcpServers }, @@ -322,24 +397,25 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { } catch (error) { if (mounted.current) toast.error(copy.errors.import, settingsActionErrorMessage(error, locale)); } finally { + for (const serverId of claimed) serverOpsRef.current.release(serverId); if (mounted.current) setBusy(null); } } async function toggle(serverId: string, server: McpServerConfig, enabled: boolean) { - setBusy(`toggle:${serverId}`); + if (!serverOpsRef.current.claim(serverId, 'toggle')) return; try { const next = await window.maka.mcp.upsert(serverId, { ...server, enabled }); if (mounted.current) setConfig(next); } catch (error) { if (mounted.current) toast.error(copy.errors.update, settingsActionErrorMessage(error, locale)); } finally { - if (mounted.current) setBusy(null); + serverOpsRef.current.release(serverId); } } async function testServer(serverId: string) { - setBusy(`test:${serverId}`); + if (!serverOpsRef.current.claim(serverId, 'test')) return; try { const result = await window.maka.mcp.test(serverId); if (!mounted.current) return; @@ -349,7 +425,44 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { } catch (error) { if (mounted.current) toast.error(copy.errors.test, settingsActionErrorMessage(error, locale)); } finally { - if (mounted.current) setBusy(null); + serverOpsRef.current.release(serverId); + } + } + + async function login(serverId: string) { + if (!serverOpsRef.current.claim(serverId, 'login')) return; + try { + const status = await window.maka.mcp.login(serverId); + if (!mounted.current) return; + setStatuses((current) => replaceStatus(current, status)); + if (status.state === 'connected') { + toast.success(copy.toast.loginOk(serverId), copy.row.tools(status.toolCount)); + } else { + toast.error(copy.errors.login, status.error ?? copy.errors.unavailableStatus); + } + } catch (error) { + // The user's own cancel ends the round with a rejection; announcing + // their click back at them as a login failure is noise. + const cancelled = error instanceof Error && /cancelled/iu.test(error.message); + if (mounted.current && !cancelled) { + toast.error(copy.errors.login, settingsActionErrorMessage(error, locale)); + } + } finally { + serverOpsRef.current.release(serverId); + } + } + + async function logout(serverId: string) { + if (!serverOpsRef.current.claim(serverId, 'logout')) return; + try { + const status = await window.maka.mcp.logout(serverId); + if (!mounted.current) return; + setStatuses((current) => replaceStatus(current, status)); + toast.success(copy.toast.loggedOut(serverId)); + } catch (error) { + if (mounted.current) toast.error(copy.errors.logout, settingsActionErrorMessage(error, locale)); + } finally { + serverOpsRef.current.release(serverId); } } @@ -363,8 +476,8 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { // The 删除 button is about to unmount with the whole inspector, and // nothing else would claim focus — hand it to the row that takes the // deleted one's place. + if (!serverOpsRef.current.claim(serverId, 'remove')) return; focusRowAfterRemovalRef.current = installedEntries.findIndex(([id]) => id === serverId); - setBusy(`remove:${serverId}`); try { const next = await window.maka.mcp.remove(serverId); if (!mounted.current) return; @@ -375,9 +488,13 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { setSelectedServerId((current) => (current === serverId ? null : current)); toast.success(copy.toast.removed); } catch (error) { + // The row survived: disarm the pending focus move, or a later + // unrelated refresh would yank focus to the row that WOULD have + // followed the deletion while the user is reading the error. + focusRowAfterRemovalRef.current = null; if (mounted.current) toast.error(copy.errors.remove, settingsActionErrorMessage(error, locale)); } finally { - if (mounted.current) setBusy(null); + serverOpsRef.current.release(serverId); } } @@ -509,11 +626,11 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { label={serverId} description={( - {/* Exceptional state leads as TEXT; the healthy label - rides the dot's accessible name only. */} - {state.exception ? {state.label} · : null} + {/* Exceptional or actionable state leads as TEXT; the + healthy label rides the dot's accessible name only. */} + {state.exception || state.tone === 'warning' ? {state.label} · : null} {transportLabel} · {endpoint} - {state.tone === 'success' ? · {state.label} : null} + {state.tone === 'success' && status ? · {copy.row.tools(status.toolCount)} : null} )} startContent={( @@ -550,12 +667,15 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { serverId={selectedServer[0]} server={selectedServer[1]} status={statusById.get(selectedServer[0])} - busy={busy} + action={serverOps.get(selectedServer[0])} copy={copy} onToggle={(enabled) => void toggle(selectedServer[0], selectedServer[1], enabled)} onEdit={() => openEdit(selectedServer[0], selectedServer[1])} onTest={() => void testServer(selectedServer[0])} onRemove={() => void remove(selectedServer[0])} + onLogin={() => void login(selectedServer[0])} + onCancelLogin={() => void window.maka.mcp.cancelLogin(selectedServer[0]).catch(() => {})} + onLogout={() => void logout(selectedServer[0])} /> ) : undefined} actions={ @@ -636,21 +756,40 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { if (changedKey === undefined) { return {}; } - if (Object.keys(current).length === 0 || next.mode !== 'manual') { + if (next.mode !== 'manual') { return current; } + const validation = validateMcpEditorDraft(next.draft, { + existingIds: next.editingId ? undefined : Object.keys(config.mcpServers), + hasOAuth: Boolean(next.draft.oauth), + }); + // Live validation means LIVE: every substantive error — a + // colliding id, an invalid/insecure URL, embedded credentials, + // an unbalanced quote — surfaces the moment it is typed. + // Only presence errors stay save-triggered: flagging a field + // as missing while the user has not reached it yet is nagging. + if (Object.keys(current).length === 0) { + const immediate: McpEditorErrors = {}; + for (const [field, code] of Object.entries(validation) as Array< + [keyof McpEditorErrors, McpEditorValidationCode] + >) { + if (code !== 'required') immediate[field] = code; + } + return immediate; + } if (changedKey === 'kind') { - return validateMcpEditorDraft(next.draft); + return validation; } if ( changedKey !== 'id' && changedKey !== 'commandLine' && - changedKey !== 'url' + changedKey !== 'url' && + changedKey !== 'headers' ) { return current; } const nextErrors = { ...current }; - const changedError = validateMcpEditorDraft(next.draft)[changedKey]; + const changedError = validation[changedKey]; if (changedError) { nextErrors[changedKey] = changedError; } else { @@ -702,15 +841,24 @@ function McpServerInspector(props: { serverId: string; server: McpServerConfig; status?: McpServerStatus; - busy: string | null; + action: McpServerOpAction | undefined; copy: McpCopy; onToggle(enabled: boolean): void; onEdit(): void; onTest(): void; onRemove(): void; + onLogin(): void; + onCancelLogin(): void; + onLogout(): void; }) { const { serverId, server, status, copy } = props; const state = presentStatus(status, server.enabled !== false, copy); + const needsAuth = status?.state === 'needs-auth'; + // One operation per server: while any of these runs — a login parked on + // the browser callback especially — the sibling mutations stay disabled + // rather than racing it against a reconnected or deleted server. + const action = props.action; + const locked = action !== undefined; const endpoint = endpointFor(server); const transportLabel = isMcpStdioConfig(server) ? copy.page.localStdio @@ -718,15 +866,15 @@ function McpServerInspector(props: { const negotiatedProtocol = presentMcpNegotiatedProtocol(status, copy); return ( + {/* Same shape as the Skill inspector (the incident-console archetype): + identity + status, the actions that change it, then its facts. + The endpoint states itself once — in the facts list. */} {state.label} {serverId} - - {endpoint} - @@ -734,36 +882,72 @@ function McpServerInspector(props: { + {/* Busy buttons take the Astryx isLoading contract whole (DESIGN.md + §10): spinner, aria-busy and the announcement come from the prop — + no hand-swapped labels or disable-plus-spinner recreations. */} + {needsAuth ? ( +