diff --git a/apps/desktop/package.json b/apps/desktop/package.json index d55fdf028a..b39e0c2c79 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -64,6 +64,7 @@ "@fontsource-variable/geist": "^5.3.0", "@fontsource-variable/geist-mono": "^5.3.0", "@maka/ui": "0.1.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-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..24a54d610f --- /dev/null +++ b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts @@ -0,0 +1,30 @@ +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:cancelLogin', + 'mcp:logout', + ]) { + assert.match(preloadSource, new RegExp(`invokeSelectedRuntimeHost\\(host, '${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 9be7269fee..967f0944eb 100644 --- a/apps/desktop/src/main/runtime-host-boot.ts +++ b/apps/desktop/src/main/runtime-host-boot.ts @@ -34,12 +34,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"; @@ -79,7 +82,7 @@ import { showMessageBoxWithDiagnostics } from "./native-diagnostic-dialog.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 { @@ -277,6 +280,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 { @@ -815,6 +835,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) @@ -929,6 +976,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 8eed5626c0..ce4d2ac128 100644 --- a/apps/desktop/src/preload/bridge-contract.d.ts +++ b/apps/desktop/src/preload/bridge-contract.d.ts @@ -102,6 +102,7 @@ import type { DesktopDiagnosticInput } from './diagnostics-contract.js'; import type { Result } from '@maka/core/result'; import type { CreateSessionRequestInput } from '@maka/core/runtime-inputs'; import type { + McpConfigAddResult, McpConfigFile, McpServerConfig, McpServerStatus, @@ -811,11 +812,18 @@ export interface MakaBridge { getConfig(host?: DesktopRuntimeHostRef): Promise; listStatuses(host?: DesktopRuntimeHostRef): Promise; setConfig(config: McpConfigFile, host?: DesktopRuntimeHostRef): 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, host?: DesktopRuntimeHostRef): Promise; upsert(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise; install(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise; remove(serverId: string, host?: DesktopRuntimeHostRef): Promise; cancelInstall(serverId: string, host?: DesktopRuntimeHostRef): Promise; test(serverId: string, host?: DesktopRuntimeHostRef): Promise; + login(serverId: string, host?: DesktopRuntimeHostRef): Promise; + /** Ends an in-flight login round; resolves false when none is active. */ + cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise; + logout(serverId: string, host?: DesktopRuntimeHostRef): 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 231dacbf2b..53a00ea5de 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -156,6 +156,7 @@ import { import type { Result } from '@maka/core/result'; import type { CreateSessionRequestInput } from '@maka/core/runtime-inputs'; import type { + McpConfigAddResult, McpConfigFile, McpServerConfig, McpServerStatus, @@ -2096,6 +2097,9 @@ const makaBridge = { setConfig(config: McpConfigFile, host?: DesktopRuntimeHostRef): Promise { return invokeSelectedRuntimeHost(host, 'mcp:setConfig', config); }, + add(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise { + return invokeSelectedRuntimeHost(host, 'mcp:add', serverId, config); + }, upsert(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise { return invokeSelectedRuntimeHost(host, 'mcp:upsert', serverId, config); }, @@ -2111,6 +2115,19 @@ const makaBridge = { test(serverId: string, host?: DesktopRuntimeHostRef): Promise { return invokeSelectedRuntimeHost(host, '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, host?: DesktopRuntimeHostRef): Promise { + return invokeSelectedRuntimeHost(host, 'mcp:login', serverId); + }, + cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise { + return invokeSelectedRuntimeHost(host, 'mcp:cancelLogin', serverId); + }, + logout(serverId: string, host?: DesktopRuntimeHostRef): Promise { + return invokeSelectedRuntimeHost(host, 'mcp:logout', serverId); + }, subscribeChanges(handler: (statuses: McpServerStatus[]) => void): () => void { return subscribeActiveRuntimeHostEvent('mcp:changed', handler); }, diff --git a/package-lock.json b/package-lock.json index eabeb432e3..e463fe1fc6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -65,6 +65,7 @@ "@fontsource-variable/geist": "^5.3.0", "@fontsource-variable/geist-mono": "^5.3.0", "@maka/ui": "0.1.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/packages/cli/src/runtime-host-capability-provider-command.ts b/packages/cli/src/runtime-host-capability-provider-command.ts index 353a817e43..dae84fd6c4 100644 --- a/packages/cli/src/runtime-host-capability-provider-command.ts +++ b/packages/cli/src/runtime-host-capability-provider-command.ts @@ -1,15 +1,15 @@ import { createHash } from 'node:crypto'; import { readFile } from 'node:fs/promises'; import { homedir } from 'node:os'; -import { join, resolve } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; import type { McpBoundTool, McpCallResult, McpToolBinding, McpToolDescriptor, } from '@maka/core/mcp'; -import { McpClientManager } from '@maka/mcp'; -import { normalizeMcpConfig } from '@maka/storage'; +import { createCredentialMcpOAuthStorage, McpClientManager } from '@maka/mcp'; +import { createFileCredentialStore, normalizeMcpConfig } from '@maka/storage'; import { connectRemoteRuntimeHost, loadOrCreateRuntimeHostClientInstanceId, @@ -67,6 +67,11 @@ export async function runRuntimeHostCapabilityProviderCli( const manager = new McpClientManager({ clientName: 'maka-capability-provider', excludedStdioEnvironmentKeys: [credentialEnv], + // Same credential store Desktop writes (credentials.json beside the + // config): without it this process is credential-blind — every remote + // OAuth server 401s forever while forgetServerCredentials reports + // success and erases nothing. + oauthStorage: createCredentialMcpOAuthStorage(createFileCredentialStore(dirname(configPath))), }); await manager.sync(config); diff --git a/packages/core/src/mcp.ts b/packages/core/src/mcp.ts index c15558966c..50a905998e 100644 --- a/packages/core/src/mcp.ts +++ b/packages/core/src/mcp.ts @@ -42,7 +42,65 @@ export interface McpConfigFile { mcpServers: Record; } -export type McpConnectionState = 'disabled' | 'disconnected' | 'connecting' | 'connected' | 'error'; +/** The one definition of "traffic that never leaves this machine" — the + * only place cleartext http is acceptable for MCP endpoints, OAuth + * endpoints, and redirect hops. Storage validation, the runtime's fetch + * guard, the desktop OAuth controller and the editor's field validation + * all share it so the rule cannot drift. */ +export function isLoopbackHost(hostname: string): boolean { + // Only names whose loopback-ness the RUNTIME guarantees: `localhost` and + // the literal loopback addresses. `*.localhost` is loopback per RFC 6761 + // §6.3, but Node hands it to the system resolver — under an attacker's + // resolver (or hosts file) the name can point anywhere, and everything + // built on this predicate (cleartext trust, provenance roots) would + // follow it off the machine. + return ( + hostname === 'localhost' || hostname === '[::1]' || /^127(?:\.\d{1,3}){3}$/u.test(hostname) + ); +} + +/** Private-range and link-local IP LITERALS (RFC 1918, RFC 3927/4291, + * CGNAT). Hostname-based checks are deliberately out of scope: they would + * need a resolve here and could still re-resolve differently at request + * time — callers treat privately-RESOLVING names as accepted risk. */ +export function isPrivateRangeHost(hostname: string): boolean { + const v4 = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/u.exec(hostname); + if (v4) { + const [a, b] = [Number(v4[1]), Number(v4[2])]; + if (a === 10) return true; + if (a === 172 && b >= 16 && b <= 31) return true; + if (a === 192 && b === 168) return true; + if (a === 169 && b === 254) return true; + if (a === 100 && b >= 64 && b <= 127) return true; + return false; + } + if (hostname.startsWith('[')) { + const inner = hostname.slice(1, -1).toLowerCase(); + return inner.startsWith('fc') || inner.startsWith('fd') || inner.startsWith('fe8'); + } + return false; +} + +/** The composed rule the config store enforces, the runtime's fetch guard + * re-checks per hop, and the editor mirrors onto the URL field: cleartext + * http is only acceptable where it never leaves the machine. One + * definition, so the three sites cannot drift. */ +export function isNonLoopbackCleartextHttp(url: URL): boolean { + return url.protocol === 'http:' && !isLoopbackHost(url.hostname); +} + +/** Result of adding a new server. A taken id is an expected dialog outcome, + * so it travels as data the renderer can switch on rather than as prose + * fished out of a flattened IPC error string. */ +export type McpConfigAddResult = { status: 'added'; config: McpConfigFile } | { status: 'exists' }; + +export type McpConnectionState = + | 'disabled' + | 'disconnected' + | 'connecting' + | 'connected' + | 'needs-auth' + | 'error'; export interface McpNegotiatedProtocol { era: 'legacy' | 'modern'; @@ -95,6 +153,9 @@ export interface McpServerStatus { tools: McpToolDescriptor[]; error?: string; stderrTail?: string[]; + /** True when the connection is backed by stored OAuth credentials — + * the UI offers logout only where there is something to drop. */ + authenticated?: boolean; updatedAt: number; } diff --git a/packages/mcp/src/__fixtures__/stdio-server.ts b/packages/mcp/src/__fixtures__/stdio-server.ts index fe9d754d63..1632bff9ab 100644 --- a/packages/mcp/src/__fixtures__/stdio-server.ts +++ b/packages/mcp/src/__fixtures__/stdio-server.ts @@ -82,7 +82,10 @@ server.setRequestHandler(CallToolRequestSchema, async ({ params }) => { }; } if (params.name === 'fail') { - return { isError: true, content: [{ type: 'text', text: 'deliberate failure' }] }; + // With FIXTURE_LEAK_TOKEN set, behaves like a server echoing a secret + // it was handed — the manager must scrub it out of the tool error. + const leak = process.env.FIXTURE_LEAK_TOKEN ? ` ${process.env.FIXTURE_LEAK_TOKEN}` : ''; + return { isError: true, content: [{ type: 'text', text: `deliberate failure${leak}` }] }; } if (params.name === 'slow') { await new Promise((resolve) => setTimeout(resolve, 30_000)); diff --git a/packages/mcp/src/__tests__/credential-coordinator.test.ts b/packages/mcp/src/__tests__/credential-coordinator.test.ts new file mode 100644 index 0000000000..f47e82e21a --- /dev/null +++ b/packages/mcp/src/__tests__/credential-coordinator.test.ts @@ -0,0 +1,149 @@ +import assert from 'node:assert/strict'; +import { describe, test } from 'node:test'; +import { McpCredentialCoordinator } from '../credential-coordinator.js'; +import type { McpOAuthRecord, McpOAuthStorage } from '../oauth.js'; + +describe('McpCredentialCoordinator', () => { + test('an abort landing during the storage read blocks the commit', async () => { + // The guard is checked before the read; without the re-check after it, + // an abort arriving while the read waits would still commit the stale + // record — a timed-out OAuth flow persisting its verifier late. + let releaseGet!: () => void; + const gate = new Promise((resolve) => { + releaseGet = resolve; + }); + const writes: McpOAuthRecord[] = []; + const storage: McpOAuthStorage = { + get: async () => { + await gate; + return undefined; + }, + set: async (_id, record) => { + writes.push(record); + }, + delete: async () => {}, + }; + const coordinator = new McpCredentialCoordinator(storage); + const round = new AbortController(); + const flow = coordinator.flowStorage('remote', { signal: round.signal }); + + const write = flow.set('remote', { codeVerifier: 'late-verifier' }); + round.abort(); + releaseGet(); + + await assert.rejects(write, /abandoned/u); + assert.deepEqual(writes, []); + }); + + test('a stale flow cannot delete or overwrite another writer’s rotation', async () => { + // Process A's flow reads the record, process B rotates it (its own + // coordinator, same backing store), then A's failing flow tries to + // invalidate. Generations match — only the version fence can tell A + // that the material it wants to delete is not the material it read. + const backing = new Map(); + const storage: McpOAuthStorage = { + get: async (id) => backing.get(id) && structuredClone(backing.get(id)), + set: async (id, record) => { + backing.set(id, structuredClone(record)); + }, + delete: async (id) => { + backing.delete(id); + }, + }; + await storage.set('remote', { version: 1, tokens: { access_token: 'r1', token_type: 'B' } }); + + const coordinatorA = new McpCredentialCoordinator(storage); + const coordinatorB = new McpCredentialCoordinator(storage); + const flowA = coordinatorA.flowStorage('remote', {}); + await flowA.get('remote'); // A captures version 1 + + const flowB = coordinatorB.flowStorage('remote', {}); + await flowB.get('remote'); + await flowB.set('remote', { tokens: { access_token: 'r2-rotated', token_type: 'B' } }); + + await assert.rejects(flowA.delete('remote'), /rotated by another writer/u); + await assert.rejects( + flowA.set('remote', { tokens: { access_token: 'r1-stale', token_type: 'B' } }), + /rotated by another writer/u, + ); + assert.equal(backing.get('remote')?.tokens?.access_token, 'r2-rotated'); + }); + + test('a logout landing during the storage read blocks the commit too', async () => { + let releaseGet!: () => void; + const gate = new Promise((resolve) => { + releaseGet = resolve; + }); + let held = true; + const writes: McpOAuthRecord[] = []; + const storage: McpOAuthStorage = { + get: async () => { + if (held) { + held = false; + await gate; + } + return undefined; + }, + set: async (_id, record) => { + writes.push(record); + }, + delete: async () => {}, + }; + const coordinator = new McpCredentialCoordinator(storage); + const flow = coordinator.flowStorage('remote', {}); + + const write = flow.set('remote', { codeVerifier: 'late-verifier' }); + const erasing = coordinator.erase('remote'); + releaseGet(); + await erasing.catch(() => {}); + + await assert.rejects(write, /cleared/u); + // Only the tombstone landed; the flow's write never did. + assert.equal(writes.length, 1); + assert.equal(writes[0]?.codeVerifier, undefined); + assert.equal(writes[0]?.generation, 1); + }); + + test('an abandoned erase cannot tombstone the record a newer login stored', async () => { + // A timed-out logout abandons its caller, but the stalled erase keeps + // running. When it resumes it would read the CURRENT record — the fresh + // tokens a newer login just stored — as its basis and tombstone them. + // The abandonment signal fences the commit on both sides of the read. + let releaseGet!: () => void; + const gate = new Promise((resolve) => { + releaseGet = resolve; + }); + let stored: McpOAuthRecord = { version: 1, generation: 0 }; + const writes: McpOAuthRecord[] = []; + const storage: McpOAuthStorage = { + get: async () => { + await gate; + return stored; + }, + set: async (_id, record) => { + writes.push(record); + stored = record; + }, + delete: async () => {}, + }; + const coordinator = new McpCredentialCoordinator(storage); + const round = new AbortController(); + + const erasing = coordinator.erase('remote', { signal: round.signal }); + // The logout round times out while the storage read is still parked; + // a fresh login then completes and rotates the record. + round.abort(); + stored = { version: 7, generation: 0 }; + releaseGet(); + + await assert.rejects(erasing, /abandoned/u); + assert.equal(writes.length, 0); + assert.equal(stored.version, 7); + + // An already-abandoned erase never reaches storage at all. + const aborted = new AbortController(); + aborted.abort(); + await assert.rejects(coordinator.erase('remote', { signal: aborted.signal }), /abandoned/u); + assert.equal(writes.length, 0); + }); +}); diff --git a/packages/mcp/src/__tests__/manager-fallback.test.ts b/packages/mcp/src/__tests__/manager-fallback.test.ts index 9178a1a015..bb7a431dc9 100644 --- a/packages/mcp/src/__tests__/manager-fallback.test.ts +++ b/packages/mcp/src/__tests__/manager-fallback.test.ts @@ -71,7 +71,11 @@ describe('McpClientManager Streamable HTTP fallback contract', () => { await manager.sync(remoteConfig(fixture.url, 'auto')); - assert.equal(manager.status('remote')?.state, 'error', `HTTP ${status}`); + // A 401 is an authorization demand, not a dead transport: the manager + // surfaces needs-auth so the UI can offer a login instead of an error. + // Either way the probe must not fall back to legacy SSE. + const expectedState = status === 401 ? 'needs-auth' : 'error'; + assert.equal(manager.status('remote')?.state, expectedState, `HTTP ${status}`); assert.equal(fixture.streamableMethods[0], 'server/discover', `HTTP ${status}`); assert.ok(fixture.streamablePosts >= 1, `HTTP ${status}`); assert.equal(fixture.sseGets, 0, `HTTP ${status}`); @@ -226,9 +230,17 @@ describe('McpClientManager Streamable HTTP fallback contract', () => { rejection.message, 'MCP server "remote" connection failed: Streamable HTTP and legacy SSE connection attempts failed', ); - assert.ok(rejection.cause instanceof AggregateError); - assert.equal(rejection.cause.errors.length, 2); - assert.ok(rejection.cause.errors.every((error) => error instanceof Error)); + // The rejection leaves the manager (reconnect → IPC → renderer): its + // cause survives as a SANITIZED copy — the per-transport aggregate + // shape is preserved for diagnosability, but each member is rebuilt + // with a scrubbed message and no deeper chain or payload fields. + const cause = (rejection as Error & { cause?: unknown }).cause; + assert.ok(cause instanceof AggregateError); + assert.equal(cause.errors.length, 2); + for (const member of cause.errors) { + assert.ok(member instanceof Error); + assert.equal((member as Error & { cause?: unknown }).cause, undefined); + } assert.equal(fixture.streamablePosts, postsBefore + 2); assert.equal(fixture.sseGets, getsBefore + 1); diff --git a/packages/mcp/src/__tests__/manager.test.ts b/packages/mcp/src/__tests__/manager.test.ts index ff76d2d54c..8627c6d9c0 100644 --- a/packages/mcp/src/__tests__/manager.test.ts +++ b/packages/mcp/src/__tests__/manager.test.ts @@ -83,6 +83,141 @@ describe('McpClientManager E2E', { concurrency: false }, () => { assert.equal(methods.includes('initialize'), false); }); + test('strips configured headers when a redirect leaves the endpoint origin', async () => { + // Undici forwards custom headers (X-API-Key) across cross-origin + // redirects; the manager's scoped fetch must not. + const crossOriginSeen: Array<{ authorization?: string; apiKey?: string }> = []; + const target = createServer((req, res) => { + crossOriginSeen.push({ + ...(typeof req.headers.authorization === 'string' + ? { authorization: req.headers.authorization } + : {}), + ...(typeof req.headers['x-api-key'] === 'string' + ? { apiKey: req.headers['x-api-key'] as string } + : {}), + }); + res.writeHead(404, { 'content-type': 'application/json' }).end('{}'); + }); + await new Promise((resolve, reject) => { + target.once('error', reject); + target.listen(0, '127.0.0.1', resolve); + }); + const targetAddress = target.address(); + if (!targetAddress || typeof targetAddress === 'string') throw new Error('no target port'); + const redirector = createServer((req, res) => { + res + .writeHead(307, { location: `http://127.0.0.1:${targetAddress.port}${req.url ?? '/'}` }) + .end(); + }); + await new Promise((resolve, reject) => { + redirector.once('error', reject); + redirector.listen(0, '127.0.0.1', resolve); + }); + const redirectorAddress = redirector.address(); + if (!redirectorAddress || typeof redirectorAddress === 'string') + throw new Error('no redirector port'); + + try { + const manager = createManager(); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: `http://127.0.0.1:${redirectorAddress.port}/mcp`, + transport: 'streamable-http', + headers: { Authorization: 'Bearer remote-test', 'X-API-Key': 'key-123456' }, + }, + }, + }); + assert.equal(manager.status('remote')?.state, 'error'); + assert.ok(crossOriginSeen.length > 0); + for (const seen of crossOriginSeen) { + assert.equal(seen.authorization, undefined); + assert.equal(seen.apiKey, undefined); + } + } finally { + target.closeAllConnections(); + redirector.closeAllConnections(); + await Promise.all([ + new Promise((resolve) => target.close(() => resolve())), + new Promise((resolve) => redirector.close(() => resolve())), + ]); + } + }); + + test('refuses a redirect that downgrades to cleartext http off the machine', async () => { + const redirector = createServer((_req, res) => { + res.writeHead(307, { location: 'http://203.0.113.5/mcp' }).end(); + }); + await new Promise((resolve, reject) => { + redirector.once('error', reject); + redirector.listen(0, '127.0.0.1', resolve); + }); + const address = redirector.address(); + if (!address || typeof address === 'string') throw new Error('no redirector port'); + try { + const manager = createManager(); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: `http://127.0.0.1:${address.port}/mcp`, + transport: 'streamable-http', + headers: { 'X-API-Key': 'key-123456' }, + }, + }, + }); + assert.equal(manager.status('remote')?.state, 'error'); + assert.match(manager.status('remote')?.error ?? '', /cleartext|https/iu); + } finally { + redirector.closeAllConnections(); + await new Promise((resolve) => redirector.close(() => resolve())); + } + }); + + test('auto falls back to legacy SSE without replacing protocol headers', async () => { + const fixture = await createRemoteFixture('sse'); + const manager = createManager(); + await manager.sync(remoteConfig(`${fixture.url}/sse`, 'auto')); + + assert.equal(manager.status('remote')?.transport, 'sse'); + const result = await manager.callTool(bindingFor(manager, 'remote', 'echo'), { + value: 'legacy', + }); + assert.deepEqual(result.content, [{ type: 'text', text: 'legacy' }]); + const get = fixture.requests.find( + (request) => request.method === 'GET' && request.path === '/sse', + ); + assert.equal(get?.authorization, 'Bearer remote-test'); + assert.match(get?.accept ?? '', /text\/event-stream/u); + assert.ok( + fixture.requests.some( + (request) => + request.method === 'POST' && + request.path === '/messages' && + request.authorization === 'Bearer remote-test', + ), + ); + assertLegacyHandshake(fixture); + }); + + test('still sends low-level discovery when a legacy server omits tools capability', async () => { + const fixture = await createRemoteFixture('streamable-http', { + advertiseTools: false, + }); + const manager = createManager(); + + await manager.sync(remoteConfig(fixture.url)); + + assert.ok( + fixture.requests.some((request) => request.protocolMethods.includes('tools/list')), + JSON.stringify({ + status: manager.status('remote'), + methods: fixture.requests.flatMap((request) => request.protocolMethods), + }), + ); + }); + test('bounds and sanitizes connection errors before publishing status', async () => { const fixture = await createRemoteFixture('streamable-http'); fixture.setToolListMode('duplicate'); @@ -813,6 +948,44 @@ describe('McpClientManager E2E', { concurrency: false }, () => { assert.equal(countProtocolMethod(fixture, 'tools/call'), callsBefore); }); + + test('a failed tool call rejects with a sanitized cause, never the raw chain', async () => { + // The scrubbed message is only half the boundary: the normalized + // error's cause retains the RAW transport error, and an endpoint that + // reflects the Authorization header into an HTTP failure would leak + // it to any cause-aware logger or serializer past the manager. The + // retained cause keeps its typed identity but loses the raw text and + // the deeper chain. + const fixture = await createRemoteFixture('streamable-http'); + const manager = createManager(); + await manager.sync(remoteConfig(fixture.url)); + const aborted = new AbortController(); + const reflected = new Error('carrier: Bearer remote-test', { + cause: new Error('deeper: Bearer remote-test'), + }); + aborted.abort(reflected); + + let rejection: unknown; + try { + await manager.callTool( + bindingFor(manager, 'remote', 'echo'), + { value: 'x' }, + { + signal: aborted.signal, + }, + ); + } catch (error) { + rejection = error; + } + + assert.ok(rejection instanceof McpToolCallError); + assert.doesNotMatch(String((rejection as Error).message), /remote-test/u); + const cause = (rejection as Error & { cause?: unknown }).cause; + assert.ok(cause instanceof Error); + assert.notEqual(cause, reflected); + assert.doesNotMatch(cause.message, /remote-test/u); + assert.equal((cause as Error & { cause?: unknown }).cause, undefined); + }); }); describe('McpClientManager stdio E2E', () => { @@ -852,6 +1025,54 @@ describe('McpClientManager E2E', { concurrency: false }, () => { ); }); + test('a tool error echoing a configured env secret is scrubbed', async () => { + const manager = createManager(); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + fixture: { + command: process.execPath, + args: [fixturePath], + env: { FIXTURE_LEAK_TOKEN: 'env-secret-value' }, + }, + }, + }); + await assert.rejects( + manager.callTool(bindingFor(manager, 'fixture', 'fail'), {}), + (error: unknown) => { + assert.ok(error instanceof McpToolCallError); + assert.doesNotMatch(error.message, /env-secret-value/u); + assert.match(error.message, /\[redacted\]/u); + return true; + }, + ); + }); + + test('a short sensitive env value is withheld wholesale from tool errors', async () => { + const manager = createManager(); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + fixture: { + command: process.execPath, + args: [fixturePath], + // A 3-character credential cannot be spliced out; the key names + // it as a secret, so the whole echoing message must be withheld. + env: { FIXTURE_LEAK_TOKEN: 'k7#' }, + }, + }, + }); + await assert.rejects( + manager.callTool(bindingFor(manager, 'fixture', 'fail'), {}), + (error: unknown) => { + assert.ok(error instanceof McpToolCallError); + assert.doesNotMatch(error.message, /k7#/u); + assert.match(error.message, /withheld/u); + return true; + }, + ); + }); + test('propagates caller abort to an in-flight tool call', async () => { const manager = createManager(); await manager.sync(fixtureConfig()); diff --git a/packages/mcp/src/__tests__/oauth.test.ts b/packages/mcp/src/__tests__/oauth.test.ts new file mode 100644 index 0000000000..900bfa84fb --- /dev/null +++ b/packages/mcp/src/__tests__/oauth.test.ts @@ -0,0 +1,1665 @@ +import assert from 'node:assert/strict'; +import { createHash, randomUUID } from 'node:crypto'; +import { createServer, type IncomingMessage } from 'node:http'; +import { afterEach, describe, test } from 'node:test'; +import { Server as McpServer } from '@modelcontextprotocol/sdk/server/index.js'; +import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; +import { + CallToolRequestSchema, + LATEST_PROTOCOL_VERSION, + ListToolsRequestSchema, +} from '@modelcontextprotocol/sdk/types.js'; +import { MCP_CONFIG_VERSION, type McpConfigFile } from '@maka/core/mcp'; +import { + createMemoryMcpOAuthStorage, + McpClientManager, + McpOAuthProvider, + type McpOAuthRecord, + type McpOAuthStorage, +} from '../index.js'; + +// End-to-end OAuth against a real authorization server fixture: RFC 9728 +// protected-resource discovery from the 401, dynamic client registration, +// PKCE authorization-code exchange, and the authorized reconnect. The +// "browser" is a manual fetch of the authorization URL; the fixture +// auto-approves and redirects with a code, exactly like a consent screen. + +const managers: McpClientManager[] = []; +const fixtures: OAuthFixture[] = []; + +afterEach(async () => { + await Promise.all(managers.splice(0).map((manager) => manager.close())); + await Promise.all(fixtures.splice(0).map((fixture) => fixture.close())); +}); + +describe('McpClientManager OAuth E2E', () => { + test('needs-auth → authorize → connected, with PKCE and Bearer-protected calls', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + assert.equal(manager.status('remote')?.error, undefined); + // The background 401 round persisted its discovery (including the + // WWW-Authenticate resource_metadata URL), so the interactive login + // below starts from that context instead of a from-scratch discovery. + assert.ok((await storage.get('remote'))?.discovery?.authorizationServerUrl); + + const redirectUrl = 'http://127.0.0.1:39999/callback'; + const start = await manager.startAuthorization('remote', redirectUrl, { state: 'maka-state' }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const authorizationUrl = new URL(start.authorizationUrl); + assert.equal(authorizationUrl.pathname, '/authorize'); + assert.equal(authorizationUrl.searchParams.get('state'), 'maka-state'); + assert.equal(authorizationUrl.searchParams.get('redirect_uri'), redirectUrl); + // The 401 challenge's scope made it into the authorization request. + assert.equal(authorizationUrl.searchParams.get('scope'), 'files:read'); + assert.equal(authorizationUrl.searchParams.get('code_challenge_method'), 'S256'); + assert.ok(authorizationUrl.searchParams.get('code_challenge')); + // Dynamic registration ran before the redirect. + assert.ok(fixture.registrations.length >= 1); + // Consent disclosure material: the resolved issuer, the scope the round + // requests, and the round's state travel back to the caller so a UI can + // show what is being granted before a browser opens. + assert.ok(start.issuer); + assert.equal(new URL(start.issuer).origin, authorizationUrl.origin); + assert.deepEqual(start.scopes, ['files:read']); + assert.equal(start.state, 'maka-state'); + // An interactive round parks background connects: they would persist + // discovery state through the same record and trip the round's version + // fence. connect() defers instead of opening. + const versionMidRound = (await storage.get('remote'))?.version; + await manager.connect('remote'); + assert.equal((await storage.get('remote'))?.version, versionMidRound); + assert.notEqual(manager.status('remote')?.state, 'connected'); + + // The user's browser: hit the consent screen, get redirected back. + const consent = await fetch(authorizationUrl, { redirect: 'manual' }); + assert.equal(consent.status, 302); + const location = new URL(consent.headers.get('location') ?? ''); + assert.equal(`${location.protocol}//${location.host}${location.pathname}`, redirectUrl); + assert.equal(location.searchParams.get('state'), 'maka-state'); + const code = location.searchParams.get('code'); + assert.ok(code); + + const status = await manager.finishAuthorization('remote', { code, state: 'maka-state' }); + assert.equal(status.state, 'connected'); + assert.equal(status.authenticated, true); + assert.deepEqual( + await manager.callTool(bindingFor(manager, 'remote', 'echo'), { value: 'authorized' }), + { + content: [{ type: 'text', text: 'authorized' }], + structuredContent: undefined, + }, + ); + assert.ok( + fixture.mcpRequests.some( + (request) => request.authorization === `Bearer ${fixture.accessToken}`, + ), + ); + // The PKCE verifier round-tripped: the token endpoint checked S256(verifier). + assert.equal(fixture.tokenExchanges.length, 1); + assert.equal(fixture.tokenExchanges[0]?.pkceVerified, true); + + const cleared = await manager.clearAuthorization('remote'); + assert.equal(cleared.state, 'needs-auth'); + }); + + test('reconnects silently once tokens are stored', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.sync(config(fixture.mcpUrl)); + const status = manager.status('remote'); + assert.equal(status?.state, 'connected'); + assert.equal(status?.authenticated, true); + }); + + test('a background connect refreshes a stale token silently, without a browser round', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + clientInformation: { client_id: 'stored-client' }, + tokens: { + access_token: 'stale-token', + token_type: 'Bearer', + refresh_token: fixture.refreshToken, + }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.sync(config(fixture.mcpUrl)); + const status = manager.status('remote'); + assert.equal(status?.state, 'connected'); + assert.equal(status?.authenticated, true); + assert.equal((await storage.get('remote'))?.tokens?.access_token, fixture.accessToken); + }); + + test('a revoked session surfaces needs-auth on the next tool call', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'connected'); + + fixture.rotateAccessToken(); + await assert.rejects( + manager.callTool(bindingFor(manager, 'remote', 'echo'), { value: 'revoked' }), + ); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + }); + + test('a stored record issued for a different URL is dropped, never replayed', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + // An offline mcp.json edit repointed the same id: the manager never saw + // the old config, so only the record's own binding can stop the replay. + await storage.set('remote', { + serverUrl: 'https://old.example/mcp', + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + assert.equal((await storage.get('remote'))?.tokens, undefined); + assert.ok( + !fixture.mcpRequests.some((req) => req.authorization === `Bearer ${fixture.accessToken}`), + ); + }); + + test('a token endpoint reflecting the client secret does not leak it through errors', async () => { + const secret = 'super-secret-value-123'; + const fixture = await createOAuthFixture({ reflectInTokenError: secret }); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: fixture.mcpUrl, + transport: 'streamable-http', + oauth: { clientId: 'static-client', clientSecret: secret }, + }, + }, + }); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39998/callback', { + state: 'scrub-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + + await assert.rejects( + manager.finishAuthorization('remote', { code, state: 'scrub-state' }), + (error: unknown) => { + assert.ok(error instanceof Error); + assert.doesNotMatch(error.message, /super-secret-value-123/u); + assert.match(error.message, /\[redacted\]/u); + return true; + }, + ); + }); + + test('a cross-origin redirect sheds the OAuth bearer token', async () => { + const fixture = await createOAuthFixture(); + const collectorSeen: Array = []; + const collector = createServer((req, res) => { + collectorSeen.push( + typeof req.headers.authorization === 'string' ? req.headers.authorization : undefined, + ); + res.writeHead(404, { 'content-type': 'application/json' }).end('{}'); + }); + await new Promise((resolve, reject) => { + collector.once('error', reject); + collector.listen(0, '127.0.0.1', resolve); + }); + const collectorAddress = collector.address(); + if (!collectorAddress || typeof collectorAddress === 'string') + throw new Error('no collector port'); + const redirector = createServer((req, res) => { + res + .writeHead(307, { + location: `http://127.0.0.1:${collectorAddress.port}${req.url ?? '/'}`, + }) + .end(); + }); + await new Promise((resolve, reject) => { + redirector.once('error', reject); + redirector.listen(0, '127.0.0.1', resolve); + }); + const redirectorAddress = redirector.address(); + if (!redirectorAddress || typeof redirectorAddress === 'string') + throw new Error('no redirector port'); + + try { + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: `http://127.0.0.1:${redirectorAddress.port}/mcp`, + transport: 'streamable-http', + }, + }, + }); + assert.notEqual(manager.status('remote')?.state, 'connected'); + assert.ok(collectorSeen.length > 0); + for (const seen of collectorSeen) assert.equal(seen, undefined); + } finally { + collector.closeAllConnections(); + redirector.closeAllConnections(); + await Promise.all([ + new Promise((resolve) => collector.close(() => resolve())), + new Promise((resolve) => redirector.close(() => resolve())), + ]); + } + }); + + test('reconnect failures reaching the caller are scrubbed of tokens and short secrets', async () => { + const fixture = await createOAuthFixture({ + mcpFailureBody: (authorization) => `upstream rejected ${authorization ?? ''} secret=abcde`, + }); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: fixture.mcpUrl, + transport: 'streamable-http', + oauth: { clientId: 'abc', clientSecret: 'abcde' }, + }, + }, + }); + const status = manager.status('remote'); + assert.equal(status?.state, 'error'); + assert.doesNotMatch(status?.error ?? '', new RegExp(fixture.accessToken, 'u')); + assert.doesNotMatch(status?.error ?? '', /abcde/u); + + // The rejection is what mcp:reconnect forwards to the renderer. + await assert.rejects(manager.reconnect('remote'), (error: unknown) => { + assert.ok(error instanceof Error); + assert.doesNotMatch(error.message, new RegExp(fixture.accessToken, 'u')); + assert.doesNotMatch(error.message, /abcde/u); + assert.match(error.message, /\[redacted\]/u); + return true; + }); + }); + + test('the challenge scope is found even when only the initialize POST answers 401', async () => { + const fixture = await createOAuthFixture({ challengeOnPostOnly: true }); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39997/callback', { + state: 'post-only-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + assert.equal(new URL(start.authorizationUrl).searchParams.get('scope'), 'files:read'); + }); + + test('reflected id_token material is scrubbed from outbound errors', async () => { + const idToken = `idtok-${randomUUID()}`; + const fixture = await createOAuthFixture({ + mcpFailureBody: (authorization) => `refused ${authorization ?? ''} ${idToken}`, + }); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer', id_token: idToken }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + const status = manager.status('remote'); + assert.equal(status?.state, 'error'); + assert.doesNotMatch(status?.error ?? '', new RegExp(idToken, 'u')); + assert.match(status?.error ?? '', /\[redacted\]/u); + }); + + test('a message containing a short secret is withheld wholesale', async () => { + const fixture = await createOAuthFixture({ + mcpFailureBody: () => 'upstream rejected credential k7#', + }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: fixture.mcpUrl, + transport: 'streamable-http', + // A 3-character secret cannot be spliced out without shredding the + // message, so the whole message must be withheld instead. + oauth: { clientId: 'abc-client', clientSecret: 'k7#' }, + }, + }, + }); + const status = manager.status('remote'); + assert.equal(status?.state, 'error'); + assert.doesNotMatch(status?.error ?? '', /k7#/u); + assert.match(status?.error ?? '', /withheld/u); + }); + + test('a logout during a token refresh is terminal — the record is not resurrected', async () => { + let releaseRefresh!: () => void; + const refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + let markRefreshStarted!: () => void; + const refreshStarted = new Promise((resolve) => { + markRefreshStarted = resolve; + }); + const fixture = await createOAuthFixture({ + holdRefresh: () => { + markRefreshStarted(); + return refreshGate; + }, + }); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + clientInformation: { client_id: 'stored-client' }, + tokens: { + access_token: 'stale-token', + token_type: 'Bearer', + refresh_token: fixture.refreshToken, + }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + const syncing = manager.sync(config(fixture.mcpUrl)); + await refreshStarted; + // The user logs out while the refresh is in flight; when the fresh + // tokens finally arrive, the late write must be refused. + const clearing = manager.clearAuthorization('remote'); + releaseRefresh(); + await Promise.allSettled([syncing, clearing]); + + assert.equal((await storage.get('remote'))?.tokens, undefined); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + }); + + test('the probe speaks the current protocol version to strict POST-only servers', async () => { + const fixture = await createOAuthFixture({ + challengeOnPostOnly: true, + requireProtocolVersion: LATEST_PROTOCOL_VERSION, + }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39996/callback', { + state: 'strict-version-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + assert.equal(new URL(start.authorizationUrl).searchParams.get('scope'), 'files:read'); + }); + + test('a bare 401 on GET does not stop the probe from asking via POST', async () => { + const fixture = await createOAuthFixture({ bareChallengeOnGet: true }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'needs-auth'); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39995/callback', { + state: 'bare-get-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + // The GET's parameterless challenge is not an answer; the POST's is. + assert.equal(new URL(start.authorizationUrl).searchParams.get('scope'), 'files:read'); + }); + + test('success payloads and tool metadata are scrubbed of reflected credentials', async () => { + const fixture = await createOAuthFixture({ reflectAuthInProtocol: true }); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + const status = manager.status('remote'); + assert.equal(status?.state, 'connected'); + const tokenPattern = new RegExp(fixture.accessToken, 'u'); + // The server put the Bearer it received into the tool description… + assert.doesNotMatch(status?.tools[0]?.description ?? '', tokenPattern); + assert.match(status?.tools[0]?.description ?? '', /\[redacted\]/u); + + // …and into a successful result's content and structuredContent. + const result = await manager.callTool(bindingFor(manager, 'remote', 'echo'), { value: 'ok' }); + const text = result.content[0]?.type === 'text' ? result.content[0].text : ''; + assert.doesNotMatch(text, tokenPattern); + assert.match(text, /\[redacted\]/u); + // Including object KEYS: { [token]: 'present' } must not leak either. + assert.doesNotMatch(JSON.stringify(result.structuredContent), tokenPattern); + assert.ok( + Object.keys(result.structuredContent as Record).some((key) => + key.includes('[redacted]'), + ), + ); + }); + + test('a token endpoint reflecting the PKCE verifier does not leak it', async () => { + const fixture = await createOAuthFixture({ reflectVerifierInTokenError: true }); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39994/callback', { + state: 'verifier-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + const verifier = (await storage.get('remote'))?.codeVerifier; + assert.ok(verifier); + + await assert.rejects( + manager.finishAuthorization('remote', { code, state: 'verifier-state' }), + (error: unknown) => { + assert.ok(error instanceof Error); + assert.ok(!error.message.includes(verifier)); + assert.match(error.message, /\[redacted\]/u); + return true; + }, + ); + }); + + test('a refresh landing after logout cannot resurrect the record, even mid-write', async () => { + let releaseRefresh!: () => void; + const refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + let markRefreshStarted!: () => void; + const refreshStarted = new Promise((resolve) => { + markRefreshStarted = resolve; + }); + const fixture = await createOAuthFixture({ + holdRefresh: () => { + markRefreshStarted(); + return refreshGate; + }, + }); + const memory = createMemoryMcpOAuthStorage(); + // A storage whose writes take real time — the window the check-then-act + // race needs: the epoch check passes, then logout lands mid-write. The + // flags pin the interleaving: without them a loaded runner could let the + // write finish first and the test would silently stop covering the race + // its name describes. + let writeInFlight = false; + let logoutLandedMidWrite = false; + const slow: McpOAuthStorage = { + get: (id) => memory.get(id), + set: async (id, record) => { + writeInFlight = true; + await new Promise((resolve) => setTimeout(resolve, 120)); + writeInFlight = false; + await memory.set(id, record); + }, + delete: (id) => memory.delete(id), + }; + await memory.set('remote', { + serverUrl: fixture.mcpUrl, + clientInformation: { client_id: 'stored-client' }, + tokens: { + access_token: 'stale-token', + token_type: 'Bearer', + refresh_token: fixture.refreshToken, + }, + }); + const manager = new McpClientManager({ oauthStorage: slow }); + managers.push(manager); + + const syncing = manager.sync(config(fixture.mcpUrl)); + await refreshStarted; + releaseRefresh(); + // Wait until saveTokens has provably passed the guard check and entered + // its slow write — polling the flag pins the interleaving the test's + // name promises, instead of hoping a fixed sleep lands inside it. + const writeEntered = Date.now() + 5_000; + while (!writeInFlight && Date.now() < writeEntered) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + logoutLandedMidWrite = writeInFlight; + const clearing = manager.clearAuthorization('remote'); + await Promise.allSettled([syncing, clearing]); + + assert.equal(logoutLandedMidWrite, true); + assert.equal((await memory.get('remote'))?.tokens, undefined); + }); + + test('a raw 401 keeps its code through the scrubbed refreshTools rejection', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'connected'); + + fixture.rotateAccessToken(); + await assert.rejects(manager.refreshTools('remote'), (error: unknown) => { + assert.ok(error instanceof Error); + // The scrub must not strip the auth signal: the notification handler + // (and any caller) still needs to recognize the 401. + const code = (error as { code?: unknown }).code; + assert.ok(code === 401 || /Unauthorized|McpAuthRequired/u.test(error.name)); + return true; + }); + // The public refresh path is the same authorization loss as the + // notification path: the server leaves `connected` and its stale + // snapshot stops being callable, instead of surviving the throw. + assert.equal(manager.status('remote')?.state, 'needs-auth'); + assert.ok(!manager.toolSnapshot().tools.some((tool) => tool.descriptor.serverId === 'remote')); + }); + + test('a token endpoint reflecting the authorization code does not leak it', async () => { + const fixture = await createOAuthFixture({ reflectCodeInTokenError: true }); + const manager = new McpClientManager({ oauthStorage: createMemoryMcpOAuthStorage() }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39993/callback', { + state: 'code-reflect-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + + await assert.rejects( + manager.finishAuthorization('remote', { code, state: 'code-reflect-state' }), + (error: unknown) => { + assert.ok(error instanceof Error); + assert.ok(!error.message.includes(code)); + assert.match(error.message, /\[redacted\]/u); + return true; + }, + ); + }); + + test('a stored OAuth session excludes a configured Authorization header', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: fixture.mcpUrl, + transport: 'streamable-http', + // No oauth block (the store rejects that conflict outright), but a + // stored session exists: the bearer owns Authorization, and the + // configured header must not override the fresh token. + headers: { Authorization: 'Bearer stale-configured-header' }, + }, + }, + }); + assert.equal(manager.status('remote')?.state, 'connected'); + assert.ok( + fixture.mcpRequests.some((req) => req.authorization === `Bearer ${fixture.accessToken}`), + ); + assert.ok( + !fixture.mcpRequests.some((req) => req.authorization === 'Bearer stale-configured-header'), + ); + }); + + test('the callback iss parameter reaches the SDK issuer validation', async () => { + const fixture = await createOAuthFixture({ issueIss: true }); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39992/callback', { + state: 'iss-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const location = new URL(consent.headers.get('location') ?? ''); + const code = location.searchParams.get('code'); + const iss = location.searchParams.get('iss'); + assert.ok(code); + assert.ok(iss); + + // The genuine issuer passes... + const status = await manager.finishAuthorization('remote', { code, iss, state: 'iss-state' }); + assert.equal(status.state, 'connected'); + }); + + test('a forged callback iss is rejected before the code is redeemed', async () => { + const fixture = await createOAuthFixture({ issueIss: true }); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39991/callback', { + state: 'forged-iss-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + + await assert.rejects( + manager.finishAuthorization('remote', { + code, + iss: 'https://evil.example', + state: 'forged-iss-state', + }), + ); + // The mix-up defense fired: no tokens were minted for the forged issuer. + assert.equal((await storage.get('remote'))?.tokens, undefined); + }); + + test('every credential transition stamps a monotonically increasing version', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39990/callback', { + state: 'version-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + await manager.finishAuthorization('remote', { code, state: 'version-state' }); + + const record = await storage.get('remote'); + assert.ok(record?.tokens); + assert.ok(typeof record.version === 'number' && record.version >= 2); + }); + + test('a CAS-capable backend surfaces external-writer conflicts instead of clobbering', async () => { + const fixture = await createOAuthFixture(); + const memory = createMemoryMcpOAuthStorage(); + let tokenWriteConflicts = 0; + // Backend with compare-and-set: pre-token transitions (discovery, + // verifier) commit normally; the token write reports that another + // process changed the record between our read and write. + const storage: McpOAuthStorage = { + get: (serverId) => memory.get(serverId), + set: (serverId, record) => memory.set(serverId, record), + delete: (serverId) => memory.delete(serverId), + compareAndSet: async (serverId, _expectedVersion, record) => { + if (record.tokens) { + tokenWriteConflicts += 1; + return 'conflict'; + } + await memory.set(serverId, record); + return 'committed'; + }, + }; + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39990/callback', { + state: 'cas-conflict-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + // The exchange must refuse to overwrite the externally changed record — + // and the conflict proves CAS survives the manager's storage wrapper. + await assert.rejects( + manager.finishAuthorization('remote', { code, state: 'cas-conflict-state' }), + /outside/u, + ); + assert.ok(tokenWriteConflicts >= 1); + assert.equal((await memory.get('remote'))?.tokens, undefined); + }); + + test('a removed server or a changed URL forgets its stored authorization', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'connected'); + + // A URL edit must not replay the old endpoint's bearer token against the + // new one. Disabled so the sync does not try to reach the fake host. + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: 'https://changed.example/mcp', + transport: 'streamable-http', + enabled: false, + }, + }, + }); + // Erase is a tombstone, not an absence: the credentials are gone and + // the persisted generation advanced, so a raced flow in ANOTHER process + // (which cannot see this one's epoch) is fenced too. + const afterUrlChange = await storage.get('remote'); + assert.equal(afterUrlChange?.tokens, undefined); + assert.equal(afterUrlChange?.clientInformation, undefined); + assert.equal(afterUrlChange?.generation, 1); + + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + generation: 1, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + await manager.sync({ version: MCP_CONFIG_VERSION, mcpServers: {} }); + const afterRemoval = await storage.get('remote'); + assert.equal(afterRemoval?.tokens, undefined); + assert.equal(afterRemoval?.generation, 2); + }); + + test('a failed credential erase blocks the server instead of releasing it', async () => { + const fixture = await createOAuthFixture(); + const memory = createMemoryMcpOAuthStorage(); + await memory.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + let storageDown = false; + const flaky: McpOAuthStorage = { + get: (id) => memory.get(id), + set: async (id, record) => { + if (storageDown) throw new Error('credential store unavailable'); + await memory.set(id, record); + }, + delete: async (id) => { + if (storageDown) throw new Error('credential store unavailable'); + await memory.delete(id); + }, + }; + const manager = new McpClientManager({ oauthStorage: flaky }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + assert.equal(manager.status('remote')?.state, 'connected'); + + // URL change while the credential store is down: the erase is a + // prerequisite, so the new endpoint must NOT take ownership — and the + // caller that just wrote the config gets the failure instead of a + // clean resolve that hides the divergence. + storageDown = true; + await assert.rejects( + manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { url: 'https://changed.example/mcp', transport: 'streamable-http' }, + }, + }), + /credential store unavailable/u, + ); + assert.equal(manager.status('remote')?.state, 'error'); + assert.match(manager.status('remote')?.error ?? '', /could not be removed/u); + // The blocked entry is not connectable — neither the old token nor the + // new endpoint is reachable through it. + await assert.rejects(manager.connect('remote'), /blocked/u); + // The old credentials still exist (the erase failed); nothing adopted + // the new endpoint while they do. + assert.ok((await memory.get('remote'))?.tokens); + + // Removal with the store still down: the sync rejects, the entry stays + // blocked, and a same-id reconnect is refused rather than reusing the + // surviving record. + await assert.rejects( + manager.sync({ version: MCP_CONFIG_VERSION, mcpServers: {} }), + /unavailable/u, + ); + await assert.rejects(manager.connect('remote'), /blocked/u); + + // Store recovers: the next sync retires the credentials, and only then + // does the new endpoint take over. + storageDown = false; + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: 'https://changed.example/mcp', + transport: 'streamable-http', + enabled: false, + }, + }, + }); + const record = await memory.get('remote'); + assert.equal(record?.tokens, undefined); + assert.equal(record?.generation, 1); + assert.equal(manager.status('remote')?.state, 'disabled'); + }); + + test('the interactive flow never sends a configured Authorization header', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { + url: fixture.mcpUrl, + transport: 'streamable-http', + enabled: false, + headers: { Authorization: 'Bearer stale-configured-header' }, + }, + }, + }); + const before = fixture.mcpRequests.length; + + // The whole interactive round — probe, discovery, exchange — owns + // Authorization; the configured header stays out of every request. + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39994/callback', { + state: 'exclusivity-state', + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + await manager.finishAuthorization('remote', { code, state: 'exclusivity-state' }).catch(() => { + // The reconnect after the exchange may fail (server disabled); the + // requests the round itself made are what this test inspects. + }); + const during = fixture.mcpRequests.slice(before); + assert.ok(during.length > 0); + assert.ok(!during.some((req) => req.authorization === 'Bearer stale-configured-header')); + }); + + test('a logout in another process fences this process’s in-flight refresh', async () => { + let releaseRefresh!: () => void; + const refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + let markRefreshStarted!: () => void; + const refreshStarted = new Promise((resolve) => { + markRefreshStarted = resolve; + }); + const fixture = await createOAuthFixture({ + holdRefresh: () => { + markRefreshStarted(); + return refreshGate; + }, + }); + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: fixture.mcpUrl, + clientInformation: { client_id: 'stored-client' }, + tokens: { + access_token: 'stale-token', + token_type: 'Bearer', + refresh_token: fixture.refreshToken, + }, + }); + // Two managers over ONE storage = two processes. B's in-memory epoch + // never sees A's logout; only the persisted generation can fence it. + const managerB = new McpClientManager({ oauthStorage: storage }); + const managerA = new McpClientManager({ oauthStorage: storage }); + managers.push(managerB, managerA); + + const syncing = managerB.sync(config(fixture.mcpUrl)); + await refreshStarted; + const disabled = config(fixture.mcpUrl); + for (const server of Object.values(disabled.mcpServers)) server.enabled = false; + await managerA.sync(disabled); + await managerA.forgetServerCredentials('remote'); + + releaseRefresh(); + await syncing.catch(() => {}); + + // B's refresh completed at the token endpoint, but its write was + // refused: the tombstone stands and the tokens are NOT resurrected. + const record = await storage.get('remote'); + assert.equal(record?.tokens, undefined); + assert.equal(record?.generation, 1); + }); + + test('a credential record with no endpoint binding is revoked, not adopted', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + // Legacy/hand-written record: credential material, no serverUrl. + await storage.set('remote', { + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + clientInformation: { client_id: 'stored-client' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + // Provenance cannot be established after the fact: the record must not + // have been used, and it must be gone — the user logs in again. + assert.equal(manager.status('remote')?.state, 'needs-auth'); + const record = await storage.get('remote'); + assert.equal(record?.tokens, undefined); + assert.equal(record?.clientInformation, undefined); + assert.ok( + !fixture.mcpRequests.some((req) => req.authorization === `Bearer ${fixture.accessToken}`), + ); + }); + + test('a superseded authorization round cannot finish with the older verifier', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + // Round 1 starts and its browser leg completes… + const first = await manager.startAuthorization('remote', 'http://127.0.0.1:39996/callback', { + state: 'round-one', + }); + assert.equal(first.status, 'redirect'); + if (first.status !== 'redirect') return; + const firstConsent = await fetch(first.authorizationUrl, { redirect: 'manual' }); + const firstCode = new URL(firstConsent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(firstCode); + + // …but round 2 starts before round 1 finishes, overwriting the pending + // verifier. Round 1's finish must be refused — exchanging its code + // against round 2's PKCE verifier could not succeed and must not try. + const second = await manager.startAuthorization('remote', 'http://127.0.0.1:39997/callback', { + state: 'round-two', + }); + assert.equal(second.status, 'redirect'); + if (second.status !== 'redirect') return; + await assert.rejects( + manager.finishAuthorization('remote', { code: firstCode, state: 'round-one' }), + /superseded/u, + ); + + // Round 2 completes normally. + const secondConsent = await fetch(second.authorizationUrl, { redirect: 'manual' }); + const secondCode = new URL(secondConsent.headers.get('location') ?? '').searchParams.get( + 'code', + ); + assert.ok(secondCode); + const status = await manager.finishAuthorization('remote', { + code: secondCode, + state: 'round-two', + }); + assert.equal(status.state, 'connected'); + }); + + test('a discovery that moves to another authorization server drops the registered client', async () => { + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: 'https://mcp.example/mcp', + clientInformation: { client_id: 'as-a-client', client_secret: 'as-a-secret' }, + tokens: { access_token: 'as-a-token', token_type: 'Bearer' }, + discovery: { authorizationServerUrl: 'https://as-a.example' } as never, + }); + const provider = new McpOAuthProvider({ + serverId: 'remote', + serverUrl: 'https://mcp.example/mcp', + storage, + clientName: 'maka', + clientVersion: '0.0.0', + }); + await provider.saveDiscoveryState({ + authorizationServerUrl: 'https://as-b.example', + } as never); + const record = await storage.get('remote'); + // One client per issuer: AS-A's registration (and its tokens) must not + // be presented to AS-B. + assert.equal(record?.clientInformation, undefined); + assert.equal(record?.tokens, undefined); + assert.ok(record?.discovery); + }); + + test('an atomic update cannot rebind another endpoint’s material to this one', async () => { + // Offline mcp.json repoint with SAME discovery issuer: the update path + // must apply read()'s fail-closed binding to its basis, or the old + // endpoint's tokens/client/discovery ride into the re-stamped record. + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: 'https://old.example/mcp', + clientInformation: { client_id: 'old-client', client_secret: 'old-secret' }, + tokens: { access_token: 'old-token', token_type: 'Bearer' }, + discovery: { authorizationServerUrl: 'https://as.example' } as never, + generation: 3, + version: 7, + }); + const provider = new McpOAuthProvider({ + serverId: 'remote', + serverUrl: 'https://new.example/mcp', + storage: withUpdate(storage), + clientName: 'maka', + clientVersion: '0.0.0', + }); + // First write of a fresh round against the NEW endpoint (same issuer). + await provider.saveDiscoveryState({ + authorizationServerUrl: 'https://as.example', + } as never); + const record = await storage.get('remote'); + assert.equal(record?.serverUrl, 'https://new.example/mcp'); + assert.equal(record?.tokens, undefined); + assert.equal(record?.clientInformation, undefined); + assert.ok(record?.discovery); + // Coordinator bookkeeping survives the strip. + assert.equal(record?.generation, 3); + }); + + test('a logout during the pre-write probe fences the whole flow', async () => { + // Process B pins its flow, then process A logs out while B is still in + // its remote probe (before B's first write). B's later writes must be + // refused: the pinned generation predates the tombstone. + const storage = createMemoryMcpOAuthStorage(); + await storage.set('remote', { + serverUrl: 'https://mcp.example/mcp', + tokens: { access_token: 'live', token_type: 'Bearer' }, + }); + const managerB = new McpClientManager({ oauthStorage: storage }); + const managerA = new McpClientManager({ oauthStorage: storage }); + managers.push(managerB, managerA); + const config: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { url: 'https://mcp.example/mcp', transport: 'streamable-http', enabled: false }, + }, + }; + await managerB.sync(config); + await managerA.sync(config); + + // B pins its flow (beginFlow semantics), then A tombstones. + const flowB = await ( + managerB as unknown as { + beginFlow(serverId: string): Promise; + } + ).beginFlow('remote'); + await managerA.forgetServerCredentials('remote'); + + await assert.rejects( + flowB.set('remote', { + serverUrl: 'https://mcp.example/mcp', + codeVerifier: 'late-round-verifier', + }), + /revoked/u, + ); + const record = await storage.get('remote'); + assert.equal(record?.codeVerifier, undefined); + assert.equal(record?.generation, 1); + }); + + test('a callback that omits state cannot skip the round binding', async () => { + // The pending record ALWAYS carries a state (minted when the caller + // supplies none); the binding keys off the record, so an attacker- + // submitted callback with no state does not bypass verification. + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39989/callback', {}); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + // Minted and persisted even though the caller supplied none. + assert.ok(start.state); + assert.equal((await storage.get('remote'))?.pendingState, start.state); + + await assert.rejects( + manager.finishAuthorization('remote', { code: 'attacker-code' }), + /superseded by a newer login/u, + ); + }); + + test('a superseded abandon leaves the newer round’s verifier and state intact', async () => { + // Round A dies and its abandon is queued; before the clearing write + // lands, round B persists a fresh verifier/state. A's abandon must + // become a no-op instead of CAS-deleting B's pending round. + const rounds: McpOAuthRecord[] = [ + { version: 3, codeVerifier: 'round-a-verifier', pendingState: 'round-a-state' }, + { version: 4, codeVerifier: 'round-b-verifier', pendingState: 'round-b-state' }, + ]; + let stored = rounds[0] as McpOAuthRecord; + let reads = 0; + const storage: McpOAuthStorage = { + get: async () => { + reads += 1; + // The abandon's decision read sees round A; by the time its + // clearing write reads its basis, round B has landed. + if (reads === 2) stored = rounds[1] as McpOAuthRecord; + return stored; + }, + set: async (_id, record) => { + stored = record; + }, + delete: async () => {}, + }; + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + + await manager.abandonAuthorization('remote'); + + assert.equal(stored.codeVerifier, 'round-b-verifier'); + assert.equal(stored.pendingState, 'round-b-state'); + assert.equal(stored.version, 4); + }); + + test('removal erases a stale credential record even when the server became stdio', async () => { + // An offline mcp.json edit converted a credentialed remote server to + // stdio. Removing it must still retire the old endpoint's record — + // otherwise a same-id remote re-add at the old URL inherits the token. + const storage = createMemoryMcpOAuthStorage(); + await storage.set('convert', { + serverUrl: 'https://old.example/mcp', + tokens: { access_token: 'stale-token', token_type: 'Bearer' }, + }); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { convert: { command: 'node', enabled: false } }, + }); + + await manager.sync({ version: MCP_CONFIG_VERSION, mcpServers: {} }); + + const record = await storage.get('convert'); + assert.equal(record?.tokens, undefined); + assert.ok((record?.generation ?? 0) >= 1); + }); + + test('interactive authorization fails closed while credential cleanup is owed', async () => { + const fixture = await createOAuthFixture(); + const memory = createMemoryMcpOAuthStorage(); + await memory.set('remote', { + serverUrl: fixture.mcpUrl, + tokens: { access_token: fixture.accessToken, token_type: 'Bearer' }, + }); + let storageDown = false; + const flaky: McpOAuthStorage = { + get: (id) => memory.get(id), + set: async (id, record) => { + if (storageDown) throw new Error('credential store unavailable'); + await memory.set(id, record); + }, + delete: async (id) => { + if (storageDown) throw new Error('credential store unavailable'); + await memory.delete(id); + }, + }; + const manager = new McpClientManager({ oauthStorage: flaky }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + storageDown = true; + await assert.rejects( + manager.sync({ + version: MCP_CONFIG_VERSION, + mcpServers: { + remote: { url: 'https://changed.example/mcp', transport: 'streamable-http' }, + }, + }), + /credential store unavailable/u, + ); + assert.match(manager.status('remote')?.error ?? '', /could not be removed/u); + // Not just connect(): the interactive paths fail closed too. + await assert.rejects( + manager.startAuthorization('remote', 'http://127.0.0.1:39998/callback', { state: 's' }), + /blocked/u, + ); + await assert.rejects( + manager.finishAuthorization('remote', { code: 'x', state: 's' }), + /blocked/u, + ); + await assert.rejects(manager.clearAuthorization('remote'), /blocked/u); + }); + + test('an aborted round cannot exchange its code or write credentials', async () => { + const fixture = await createOAuthFixture(); + const storage = createMemoryMcpOAuthStorage(); + const manager = new McpClientManager({ oauthStorage: storage }); + managers.push(manager); + await manager.sync(config(fixture.mcpUrl)); + + const round = new AbortController(); + const start = await manager.startAuthorization('remote', 'http://127.0.0.1:39995/callback', { + state: 'abort-state', + signal: round.signal, + }); + assert.equal(start.status, 'redirect'); + if (start.status !== 'redirect') return; + const consent = await fetch(start.authorizationUrl, { redirect: 'manual' }); + const code = new URL(consent.headers.get('location') ?? '').searchParams.get('code'); + assert.ok(code); + + // The round's owner timed out and abandoned it; the late completion + // must neither exchange the code nor land any write. + round.abort(); + const exchanges = fixture.tokenExchanges.length; + await assert.rejects( + manager.finishAuthorization( + 'remote', + { code, state: 'abort-state' }, + { signal: round.signal }, + ), + /abandoned|abort/iu, + ); + assert.equal(fixture.tokenExchanges.length, exchanges); + assert.equal((await storage.get('remote'))?.tokens, undefined); + }); +}); + +/** Memory storage with the coordinator-style atomic update, so provider + * tests exercise the update path rather than the read+set fallback. */ +function withUpdate(storage: McpOAuthStorage): McpOAuthStorage { + return { + ...storage, + update: async (serverId, apply) => { + const basis = (await storage.get(serverId)) ?? {}; + const next = apply({ ...basis }); + await storage.set(serverId, next); + return next; + }, + }; +} + +function bindingFor(manager: McpClientManager, serverId: string, toolName: string) { + const bound = manager + .toolSnapshot() + .tools.find( + (tool) => tool.descriptor.serverId === serverId && tool.descriptor.name === toolName, + ); + if (!bound) throw new Error(`no binding for ${serverId}/${toolName}`); + return bound.binding; +} + +function config(url: string): McpConfigFile { + return { + version: MCP_CONFIG_VERSION, + mcpServers: { remote: { url, transport: 'streamable-http' } }, + }; +} + +interface OAuthFixture { + mcpUrl: string; + accessToken: string; + refreshToken: string; + mcpRequests: Array<{ authorization?: string }>; + registrations: unknown[]; + tokenExchanges: Array<{ pkceVerified: boolean }>; + /** Invalidates every issued access token, like a server-side revocation. */ + rotateAccessToken(): void; + close(): Promise; +} + +async function createOAuthFixture( + options: { + reflectInTokenError?: string; + /** Makes POST /mcp fail with this body — a server reflecting what it + * was sent (the Authorization header) into an error. */ + mcpFailureBody?: (authorization?: string) => string; + /** 405 on unauthenticated GET; only the initialize POST answers 401 — + * a legal Streamable HTTP shape the challenge probe must handle. */ + challengeOnPostOnly?: boolean; + /** Awaited before answering a refresh_token grant, so a test can land + * a logout in the middle of the refresh. */ + holdRefresh?: () => Promise; + /** In challengeOnPostOnly mode, 400 any initialize that does not carry + * the SDK's current protocol version — the strict-server shape that + * broke a pinned probe version. */ + requireProtocolVersion?: string; + /** GET answers 401 with a bare challenge (no parameters); only POST + * carries scope/metadata. */ + bareChallengeOnGet?: boolean; + /** The protocol server reflects the last HTTP Authorization it saw + * into tool descriptions, results and structuredContent. */ + reflectAuthInProtocol?: boolean; + /** The token endpoint reflects the code_verifier it received into + * error_description. */ + reflectVerifierInTokenError?: boolean; + /** The consent redirect carries an RFC 9207 `iss` parameter. */ + issueIss?: boolean; + /** The token endpoint reflects the authorization code it received into + * error_description. */ + reflectCodeInTokenError?: boolean; + } = {}, +): Promise { + let accessToken = `token-${randomUUID()}`; + let lastAuthorization = ''; + const refreshToken = `refresh-${randomUUID()}`; + const mcpRequests: Array<{ authorization?: string }> = []; + const registrations: unknown[] = []; + const tokenExchanges: Array<{ pkceVerified: boolean }> = []; + const pendingCodes = new Map(); + let origin = ''; + + const httpServer = createServer(async (req, res) => { + const url = new URL(req.url ?? '/', origin); + try { + if (url.pathname === '/mcp') { + const authorization = req.headers.authorization; + if (typeof authorization === 'string') lastAuthorization = authorization; + mcpRequests.push(typeof authorization === 'string' ? { authorization } : {}); + if (options.bareChallengeOnGet && req.method === 'GET') { + res.writeHead(401, { 'www-authenticate': 'Bearer realm="mcp"' }).end(); + return; + } + if (options.challengeOnPostOnly && req.method !== 'POST') { + res.writeHead(405).end(); + return; + } + if (options.requireProtocolVersion && req.method === 'POST') { + const body = (await readJsonBody(req)) as + | { method?: string; params?: { protocolVersion?: string } } + | undefined; + if ( + body?.method === 'initialize' && + body.params?.protocolVersion !== options.requireProtocolVersion + ) { + res + .writeHead(400, { 'content-type': 'application/json' }) + .end('{"error":"bad version"}'); + return; + } + if (authorization !== `Bearer ${accessToken}`) { + res + .writeHead(401, { + 'content-type': 'application/json', + 'www-authenticate': `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource", scope="files:read"`, + }) + .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, body); + return; + } + if (options.mcpFailureBody) { + res + .writeHead(500, { 'content-type': 'text/plain' }) + .end( + options.mcpFailureBody(typeof authorization === 'string' ? authorization : undefined), + ); + return; + } + if (authorization !== `Bearer ${accessToken}`) { + res + .writeHead(401, { + 'content-type': 'application/json', + 'www-authenticate': `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource", scope="files:read"`, + }) + .end(JSON.stringify({ error: 'unauthorized' })); + return; + } + if (req.method !== 'POST') { + res.writeHead(405).end(); + return; + } + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: undefined }); + const server = createProtocolServer( + options.reflectAuthInProtocol ? () => lastAuthorization : undefined, + ); + 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' && req.method === 'GET') { + json(res, { resource: `${origin}/mcp`, authorization_servers: [origin] }); + return; + } + if (url.pathname === '/.well-known/oauth-authorization-server' && req.method === 'GET') { + 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; + registrations.push(body); + json(res, { + client_id: `client-${registrations.length}`, + redirect_uris: body.redirect_uris, + token_endpoint_auth_method: 'none', + }); + return; + } + if (url.pathname === '/authorize' && req.method === 'GET') { + const challenge = url.searchParams.get('code_challenge'); + const redirectUri = url.searchParams.get('redirect_uri'); + const state = url.searchParams.get('state'); + if (!challenge || !redirectUri) { + res.writeHead(400).end('missing challenge or redirect_uri'); + return; + } + const code = `code-${randomUUID()}`; + pendingCodes.set(code, { challenge, redirectUri }); + const target = new URL(redirectUri); + target.searchParams.set('code', code); + if (state) target.searchParams.set('state', state); + if (options.issueIss) target.searchParams.set('iss', origin); + res.writeHead(302, { location: target.toString() }).end(); + return; + } + if (url.pathname === '/token' && req.method === 'POST') { + const params = new URLSearchParams(await readTextBody(req)); + if (options.reflectCodeInTokenError) { + json( + res, + { + error: 'invalid_grant', + error_description: `code rejected ${params.get('code') ?? ''}`, + }, + 400, + ); + return; + } + if (options.reflectVerifierInTokenError) { + json( + res, + { + error: 'invalid_grant', + error_description: `verifier rejected ${params.get('code_verifier') ?? ''}`, + }, + 400, + ); + return; + } + if (options.reflectInTokenError) { + // A hostile or buggy token endpoint echoing what it was sent. + json( + res, + { + error: 'invalid_grant', + error_description: `server rejected credential ${options.reflectInTokenError}`, + }, + 400, + ); + return; + } + if (params.get('grant_type') === 'refresh_token') { + if (params.get('refresh_token') !== refreshToken) { + json(res, { error: 'invalid_grant' }, 400); + return; + } + if (options.holdRefresh) await options.holdRefresh(); + json(res, { + access_token: accessToken, + token_type: 'Bearer', + expires_in: 3600, + refresh_token: refreshToken, + }); + return; + } + const pending = pendingCodes.get(params.get('code') ?? ''); + const verifier = params.get('code_verifier') ?? ''; + const hashed = createHash('sha256').update(verifier).digest('base64url'); + const pkceVerified = Boolean(pending && hashed === pending.challenge); + tokenExchanges.push({ pkceVerified }); + if (!pkceVerified) { + json(res, { error: 'invalid_grant' }, 400); + return; + } + json(res, { + access_token: accessToken, + token_type: 'Bearer', + expires_in: 3600, + refresh_token: refreshToken, + }); + 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}`; + + const fixture: OAuthFixture = { + mcpUrl: `${origin}/mcp`, + get accessToken() { + return accessToken; + }, + refreshToken, + mcpRequests, + registrations, + tokenExchanges, + rotateAccessToken: () => { + accessToken = `token-${randomUUID()}`; + }, + close: () => + new Promise((resolve, reject) => { + httpServer.closeAllConnections(); + httpServer.close((error) => (error ? reject(error) : resolve())); + }), + }; + fixtures.push(fixture); + return fixture; +} + +function createProtocolServer(reflect?: () => string): McpServer { + const server = new McpServer( + { name: 'maka-oauth-fixture', version: '1.0.0' }, + { capabilities: { tools: {} } }, + ); + server.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: [ + { + name: 'echo', + // A server echoing the credential it was just sent — into the tool + // metadata the client persists. + description: reflect ? `Echo text (${reflect()})` : 'Echo text', + inputSchema: { type: 'object', properties: { value: { type: 'string' } } }, + }, + ], + })); + server.setRequestHandler(CallToolRequestSchema, async ({ params }) => ({ + content: [ + { + type: 'text', + text: `${String(params.arguments?.value ?? '')}${reflect ? ` ${reflect()}` : ''}`, + }, + ], + structuredContent: reflect ? { reflected: reflect(), [reflect()]: 'present' } : undefined, + })); + return server; +} + +function json(res: import('node:http').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/packages/mcp/src/__tests__/transport-security.test.ts b/packages/mcp/src/__tests__/transport-security.test.ts new file mode 100644 index 0000000000..5e4edb179f --- /dev/null +++ b/packages/mcp/src/__tests__/transport-security.test.ts @@ -0,0 +1,94 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { assertTransportSecurity, urlProvenance } from '../transport-security.js'; + +describe('transport security provenance', () => { + const remoteRoot = urlProvenance(new URL('https://api.example.com/mcp')); + const loopbackRoot = urlProvenance(new URL('http://127.0.0.1:8080/mcp')); + + it('allows public https regardless of provenance', () => { + assert.doesNotThrow(() => + assertTransportSecurity(new URL('https://as.example.com/token'), remoteRoot), + ); + assert.doesNotThrow(() => + assertTransportSecurity(new URL('https://as.example.com/token'), loopbackRoot), + ); + }); + + it('refuses remotely supplied https aimed back into the machine or network', () => { + // A remote server's OAuth metadata naming an internal https destination + // is a request-forgery primitive (blind SSRF): the client would issue + // the GET/POST from inside the user's network. + for (const url of [ + 'https://127.0.0.1:8443/latest/meta-data/', + 'https://localhost:8443/admin', + 'https://169.254.169.254/latest/meta-data/', + 'https://192.168.1.1/router', + 'https://10.0.0.5/internal', + 'https://172.16.0.9/internal', + 'https://100.64.0.1/cgnat', + ]) { + assert.throws(() => assertTransportSecurity(new URL(url), remoteRoot), /refused remotely/u); + } + // A loopback trust root keeps its local machine reachable; an internal + // configured endpoint keeps its own network reachable. + assert.doesNotThrow(() => + assertTransportSecurity(new URL('https://127.0.0.1:8443/token'), loopbackRoot), + ); + const internalRoot = urlProvenance(new URL('https://10.1.2.3/mcp')); + assert.doesNotThrow(() => + assertTransportSecurity(new URL('https://10.0.0.5/token'), internalRoot), + ); + assert.throws( + () => assertTransportSecurity(new URL('https://127.0.0.1:8443/x'), internalRoot), + /refused remotely/u, + ); + // Privately-RESOLVING names are accepted risk (no resolve here): a + // hostname passes even under a remote root. + assert.doesNotThrow(() => + assertTransportSecurity(new URL('https://intranet.corp/token'), remoteRoot), + ); + }); + + it('never allows cleartext http off the machine', () => { + assert.throws( + () => assertTransportSecurity(new URL('http://api.example.com/mcp'), remoteRoot), + /non-loopback hosts require https/u, + ); + assert.throws( + () => assertTransportSecurity(new URL('http://10.0.0.5/mcp'), loopbackRoot), + /non-loopback hosts require https/u, + ); + }); + + it('refuses a remotely supplied loopback destination when the server is remote', () => { + // A remote https server redirecting (or pointing its OAuth metadata) at + // the user's own machine is a pivot, not a convenience. + assert.throws( + () => assertTransportSecurity(new URL('http://127.0.0.1:9999/steal'), remoteRoot), + /remotely supplied loopback/u, + ); + assert.throws( + () => assertTransportSecurity(new URL('http://localhost:22/'), remoteRoot), + /remotely supplied loopback/u, + ); + }); + + it('allows loopback http when the user configured a loopback trust root', () => { + assert.doesNotThrow(() => + assertTransportSecurity(new URL('http://127.0.0.1:8080/mcp'), loopbackRoot), + ); + // A local AS on a different loopback port is part of the same local + // trust decision the user already made. + assert.doesNotThrow(() => + assertTransportSecurity(new URL('http://127.0.0.1:9000/authorize'), loopbackRoot), + ); + }); + + it('refuses non-HTTP schemes outright', () => { + assert.throws( + () => assertTransportSecurity(new URL('file:///etc/passwd'), loopbackRoot), + /non-HTTP/u, + ); + }); +}); diff --git a/packages/mcp/src/credential-coordinator.ts b/packages/mcp/src/credential-coordinator.ts new file mode 100644 index 0000000000..8b9eec1440 --- /dev/null +++ b/packages/mcp/src/credential-coordinator.ts @@ -0,0 +1,239 @@ +// packages/mcp/src/credential-coordinator.ts +// +// The single owner of MCP OAuth credential state transitions. Every read, +// write and delete for a server's stored record flows through one +// per-server lane here, every record write carries a monotonically +// increasing version validated against the basis that was read, and every +// terminal erase advances a PERSISTED per-server generation — a tombstone +// that outlives the process. Together these make a complete OAuth state +// transition atomic — not merely each storage call — and make revocation +// terminal in every interleaving, including across processes: +// +// - a flow that began before a logout may finish its reads, but its writes +// are refused: in-process by the epoch, cross-process by the generation +// it captured on its first read no longer matching the tombstone's; +// - a write that began before the logout completes first — the queued +// erase lands after it (lane order); +// - a stale flow cannot delete a record a newer flow just stored (epoch +// check on delete); +// - an external writer (another process editing the backing store) trips +// the version/CAS check instead of being silently overwritten, when the +// storage backend exposes compare-and-set; +// - deletion never leaves "absent" behind: erase writes a tombstone record +// carrying only the advanced generation, so a cross-process flow cannot +// CAS against absence (`expectedVersion = null`) and resurrect +// credentials the user revoked. + +import type { McpOAuthRecord, McpOAuthStorage } from './oauth.js'; + +/** Guards one flow's writes: the epoch pins in-process revocation, the + * captured generation pins cross-process revocation, and the optional + * abort signal fences writes landing after the flow's round was abandoned + * (a timed-out login must not overwrite a newer round's state). */ +interface FlowGuard { + epoch: number; + generation?: number; + /** The record version this flow last observed or produced. A write whose + * basis carries a DIFFERENT version means another writer (a concurrent + * refresh in this or another process) rotated the record mid-flow: the + * stale flow must not overwrite — or delete — the fresh material. */ + version?: number; + signal?: AbortSignal; +} + +export class McpCredentialCoordinator { + private readonly epochs = new Map(); + private readonly lanes = new Map>(); + + constructor(private readonly storage: McpOAuthStorage) {} + + /** Serializes an operation into the server's credential lane. */ + run(serverId: string, op: () => Promise): Promise { + const previous = this.lanes.get(serverId) ?? Promise.resolve(); + const next = previous.then(op, op); + this.lanes.set( + serverId, + next.then( + () => {}, + () => {}, + ), + ); + return next; + } + + epoch(serverId: string): number { + return this.epochs.get(serverId) ?? 0; + } + + /** Terminal erase: bumps the in-process epoch first (in-flight flows in + * this process become stale immediately), then — inside the lane, behind + * any write already in progress — replaces the record with a tombstone + * whose generation is advanced by one. The tombstone is what makes the + * revocation terminal for OTHER processes: their flows captured the old + * generation and every later write verifies it against the stored one. + * A failure propagates — callers must not proceed as if the credentials + * were gone. + * + * The optional signal fences an ABANDONED erase: a logout whose round + * timed out must not resume later, adopt whatever record a newer login + * just stored as its basis, and tombstone the fresh tokens. */ + async erase(serverId: string, options: { signal?: AbortSignal } = {}): Promise { + this.epochs.set(serverId, this.epoch(serverId) + 1); + await this.run(serverId, async () => { + this.assertNotAbandoned(serverId, options.signal); + const basis = await this.storage.get(serverId); + // Re-check after the read: the abandonment may have landed while the + // storage read was in flight — committing past it would erase a + // record the caller no longer owns. + this.assertNotAbandoned(serverId, options.signal); + // Absence still gets a tombstone: a cross-process flow that captured + // generation 0 on absence must not CAS its credentials back in after + // this revocation. + const tombstone: McpOAuthRecord = { + generation: (basis?.generation ?? 0) + 1, + version: (basis?.version ?? 0) + 1, + }; + await this.commit(serverId, basis, tombstone); + }); + } + + private assertNotAbandoned(serverId: string, signal?: AbortSignal): void { + if (signal?.aborted) { + throw new Error(`MCP credential erase for "${serverId}" was abandoned before it landed`); + } + } + + /** One atomic state transition pinned to the flow guard: read the + * current record, verify the guard (epoch, captured generation, abort), + * apply, stamp the next version, and write — all inside the lane, with + * the version validated against the read basis (and, when the backend + * supports compare-and-set, against the stored bytes). */ + transition( + serverId: string, + guard: FlowGuard, + apply: (record: McpOAuthRecord) => McpOAuthRecord, + ): Promise { + return this.run(serverId, async () => { + this.assertGuard(serverId, guard); + const basis = await this.storage.get(serverId); + // Re-check after the read: the abort (or a logout) may have landed + // while the storage read was in flight, and committing past it would + // persist a verifier or token for an abandoned round. + this.assertGuard(serverId, guard); + const basisGeneration = basis?.generation ?? 0; + if (guard.generation !== undefined && basisGeneration !== guard.generation) { + throw new Error(`MCP credentials for "${serverId}" were revoked during the operation`); + } + if (guard.version !== undefined && (basis?.version ?? 0) !== guard.version) { + throw new Error( + `MCP credentials for "${serverId}" were rotated by another writer during the operation`, + ); + } + const record = apply(basis ? { ...basis } : {}); + // The generation is the coordinator's own bookkeeping: flows never + // set it, and a write never advances it — only erase() does. + record.generation = basisGeneration; + record.version = (basis?.version ?? 0) + 1; + await this.commit(serverId, basis, record); + return record; + }); + } + + /** A storage view for one auth flow, pinned to the epoch at flow start + * and to the persisted generation observed on the flow's first access. + * The SDK-facing provider talks to this; every operation lands in the + * lane, and writes/deletes are refused once the epoch or generation + * moved — or once the flow's round was aborted. */ + flowStorage(serverId: string, options: { signal?: AbortSignal } = {}): McpOAuthStorage { + const guard: FlowGuard = { epoch: this.epoch(serverId), ...options }; + const capture = (record: McpOAuthRecord | undefined) => { + guard.generation ??= record?.generation ?? 0; + guard.version ??= record?.version ?? 0; + }; + // The flow's own writes advance its version fence; anyone ELSE's write + // in between leaves the fence behind the stored version and the next + // transition refuses instead of overwriting the rotation. + const advance = (record: McpOAuthRecord) => { + guard.version = record.version ?? guard.version; + return record; + }; + return { + get: (id) => + this.run(serverId, async () => { + const record = await this.storage.get(id); + capture(record); + return record; + }), + set: (id, record) => + this.transition(id, guard, (basis) => { + capture(basis); + return { ...record }; + }) + .then(advance) + .then(() => {}), + update: (id, apply) => + this.transition(id, guard, (basis) => { + capture(basis); + return apply(basis); + }).then(advance), + // An in-flow invalidation clears credential material but is NOT a + // logout: the generation is preserved, not advanced, so the flow can + // continue (e.g. re-register a client) without tripping its own guard. + delete: (id) => + this.transition(id, guard, (basis) => { + capture(basis); + return {}; + }) + .then(advance) + .then(() => {}), + }; + } + + /** A flow view whose persisted generation/version are pinned BEFORE the + * caller performs any remote await. flowStorage alone captures them on + * the first storage access, which may come after a remote probe — a + * logout in another process during that window would let the flow adopt + * the tombstone's generation as its own starting point and keep writing. + * beginFlow closes the window with one lane-ordered read up front. */ + async beginFlow( + serverId: string, + options: { signal?: AbortSignal } = {}, + ): Promise { + const view = this.flowStorage(serverId, options); + await view.get(serverId); + return view; + } + + /** Lane-ordered read outside any flow (status displays, pending lookups). */ + read(serverId: string): Promise { + return this.run(serverId, () => this.storage.get(serverId)); + } + + private assertGuard(serverId: string, guard: FlowGuard): void { + if (guard.signal?.aborted) { + throw new Error(`MCP authorization for "${serverId}" was abandoned before the write landed`); + } + if (this.epoch(serverId) !== guard.epoch) { + throw new Error(`MCP credentials for "${serverId}" were cleared during the operation`); + } + } + + private async commit( + serverId: string, + basis: McpOAuthRecord | undefined, + record: McpOAuthRecord, + ): Promise { + if (this.storage.compareAndSet) { + const result = await this.storage.compareAndSet(serverId, basis?.version ?? null, record); + if (result !== 'committed') { + // Someone outside this coordinator changed the backing store + // between our read and write. Fail closed rather than clobber. + throw new Error( + `MCP credentials for "${serverId}" changed outside the coordinator (${result})`, + ); + } + return; + } + await this.storage.set(serverId, record); + } +} diff --git a/packages/mcp/src/credential-oauth-storage.ts b/packages/mcp/src/credential-oauth-storage.ts new file mode 100644 index 0000000000..32d6cd17c4 --- /dev/null +++ b/packages/mcp/src/credential-oauth-storage.ts @@ -0,0 +1,91 @@ +// packages/mcp/src/credential-oauth-storage.ts +// +// Backs MCP OAuth persistence with the app's shared credential store: one +// `mcp:` slug per server, the whole record as one JSON secret. +// Tokens therefore live in credentials.json (0600, workspace root) beside +// every other secret in the app — never in mcp.json, which is the shareable +// config file. +// +// Lives HERE, not in the Electron app: it has no Electron dependency, and +// every process that constructs an McpClientManager over the same mcp.json +// (Desktop main, the CLI capability provider) must be able to read the +// credentials Desktop wrote — a manager without oauthStorage silently +// reports pendingAuthorization()=undefined and erases nothing on +// forgetServerCredentials. + +import type { McpOAuthRecord, McpOAuthStorage } from './oauth.js'; + +/** The slice of @maka/storage's CredentialStore this adapter needs, stated + * structurally so @maka/mcp does not depend on @maka/storage. */ +export interface McpCredentialSecretStore { + getSecret(slug: string, kind: 'oauth_token'): Promise; + setSecret(slug: string, kind: 'oauth_token', value: string): Promise; + deleteSecret(slug: string, kind?: 'oauth_token'): Promise; + compareAndSetSecret?( + slug: string, + kind: 'oauth_token', + expected: string | null, + value: string, + ): Promise<{ committed: true } | { committed: false; current: string | null }>; +} + +const KIND = 'oauth_token' as const; + +export function createCredentialMcpOAuthStorage(store: McpCredentialSecretStore): McpOAuthStorage { + return { + async get(serverId) { + const raw = await store.getSecret(slug(serverId), KIND); + if (raw === null) return undefined; + try { + return JSON.parse(raw) as McpOAuthRecord; + } catch { + // A corrupt record is indistinguishable from none: the user + // re-authorizes, and the next save overwrites it. + return undefined; + } + }, + async set(serverId, record) { + await store.setSecret(slug(serverId), KIND, JSON.stringify(record)); + }, + async delete(serverId) { + await store.deleteSecret(slug(serverId), KIND); + }, + // Maps the coordinator's version-keyed CAS onto the CredentialStore's + // raw compare-and-set, so a writer outside this process (another Maka + // instance, a hand edit) trips a conflict instead of being clobbered. + ...(store.compareAndSetSecret + ? { + async compareAndSet( + serverId: string, + expectedVersion: number | null, + record: McpOAuthRecord, + ): Promise<'committed' | 'conflict' | 'gone'> { + const raw = await store.getSecret(slug(serverId), KIND); + const basis = raw === null ? undefined : parseRecord(raw); + if ((basis?.version ?? null) !== expectedVersion) return 'conflict'; + const result = await store.compareAndSetSecret?.( + slug(serverId), + KIND, + raw, + JSON.stringify(record), + ); + if (!result) return 'conflict'; + if (result.committed) return 'committed'; + return result.current === null ? 'gone' : 'conflict'; + }, + } + : {}), + }; +} + +function parseRecord(raw: string): McpOAuthRecord | undefined { + try { + return JSON.parse(raw) as McpOAuthRecord; + } catch { + return undefined; + } +} + +function slug(serverId: string): string { + return `mcp:${serverId}`; +} diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index f0ee5f9f35..a1e1287243 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -1,6 +1,9 @@ import { randomBytes } from 'node:crypto'; import { + auth, Client, + extractWWWAuthenticateParams, + LATEST_PROTOCOL_VERSION, SdkErrorCode, SdkHttpError, SSEClientTransport, @@ -12,8 +15,18 @@ import { type VersionNegotiationOptions, } from '@modelcontextprotocol/client'; import { StdioClientTransport } from '@modelcontextprotocol/client/stdio'; +import { redactSecrets } from '@maka/core/redaction'; +import { + deepScrubMcpSecrets, + EMPTY_MCP_SECRET_INVENTORY, + MCP_SECRET_MIN_SUBSTITUTION_LENGTH, + mcpSecretInventory, + scrubKnownMcpSecrets, + type McpSecretInventory, +} from '@maka/core/mcp-secrets'; import { isMcpStdioConfig, + isNonLoopbackCleartextHttp, resolveMcpRemoteProtocolPreference, type McpBoundTool, type McpCallResult, @@ -44,8 +57,48 @@ import { createMcpToolBinding, parseMcpToolBinding } from './tool-binding.js'; import { McpToolCallError, normalizeToolCallError } from './tool-call-error.js'; import { discoverMcpTools, type McpDiscoveredTool } from './tool-discovery.js'; import { McpToolCallPreparer, type McpToolCallPreparationState } from './tool-output-validation.js'; +import { McpCredentialCoordinator } from './credential-coordinator.js'; +import { + assertTransportSecurity, + urlProvenance, + type UrlProvenance, +} from './transport-security.js'; +import { + McpAuthRequiredError, + McpOAuthProvider, + type McpOAuthRecord, + type McpOAuthStorage, +} from './oauth.js'; export { McpToolCallError } from './tool-call-error.js'; +export { + createCredentialMcpOAuthStorage, + type McpCredentialSecretStore, +} from './credential-oauth-storage.js'; +export { + createMemoryMcpOAuthStorage, + McpAuthRequiredError, + McpOAuthProvider, + type McpOAuthRecord, + type McpOAuthStorage, +} from './oauth.js'; + +export type McpAuthorizationStart = + | { status: 'authorized' } + | { + status: 'redirect'; + authorizationUrl: string; + /** The round's state parameter — minted here when the caller did not + * supply one. finishAuthorization verifies the callback against it. */ + state: string; + /** The authorization server this round resolved — the origin the user + * is about to grant. Chosen by the (untrusted) MCP server's metadata, + * which is exactly why the consumer must show it. */ + issuer: string; + /** The scope this round will request, from the server's challenge or + * the configured list. */ + scopes: string[]; + }; const DEFAULT_TIMEOUTS = { remoteConnectMs: 30_000, @@ -61,6 +114,11 @@ const MAX_SUMMARIZED_ERROR_BLOCKS = 100; const OVERSIZED_TOOL_ERROR_CONTENT = 'server returned oversized error content'; const MAX_TOOL_REFRESH_PASSES = 3; const TOOL_REFRESH_BURST_IDLE_MS = 1_000; +// Recency-bounded per-server scrub material: enough to cover every value a +// server could still realistically reflect (current + a few rotations of +// access/refresh/id token, client secret, verifier), small enough that +// deepScrub stays O(bound) per payload for the life of the process. +const MAX_HARVESTED_SECRETS_PER_SERVER = 40; export interface McpClientManagerOptions { clientName?: string; @@ -68,6 +126,13 @@ export interface McpClientManagerOptions { timeouts?: Partial; now?: () => number; excludedStdioEnvironmentKeys?: readonly string[]; + /** + * Enables OAuth for remote servers. Background connects use stored tokens + * (refreshing silently when possible) and report `needs-auth` instead of + * `error` when the server demands an interactive round; the interactive + * flow itself runs through startAuthorization/finishAuthorization. + */ + oauthStorage?: McpOAuthStorage; } export type McpManagerChangeListener = (status: McpServerStatus) => void; @@ -128,6 +193,10 @@ interface ToolRefreshNotificationState { interface Connection { config: McpServerConfig; fingerprint: string; + /** Set when stored credentials for this entry's (old) config could not be + * erased. The entry is blocked — connect() refuses it — until a later + * sync retires the credentials; only then may a new config take over. */ + credentialCleanupOwed?: McpServerConfig; client?: Client; transport?: Transport; stdioTransport?: StdioClientTransport; @@ -168,6 +237,12 @@ interface McpClientEventBridge { export class McpClientManager { private readonly connections = new Map(); + /** Servers with an interactive OAuth round in flight (startAuthorization + * → finish/abandon). A background connect is a WRITER against the same + * credential record — the SDK persists discovery state on every auth + * pass — and racing one against the round trips the round's version + * fence, failing the user's login over nothing they did. */ + private readonly interactiveRounds = new Set(); private bindingIndex = new Map(); private readonly listeners = new Set(); private syncQueue: Promise = Promise.resolve(); @@ -177,6 +252,7 @@ export class McpClientManager { private readonly clientName: string; private readonly clientVersion: string; private readonly excludedStdioEnvironmentKeys: readonly string[]; + private readonly oauthStorage?: McpOAuthStorage; private readonly toolCallPreparer = new McpToolCallPreparer(); private readonly bindingManagerId = randomBytes(16).toString('base64url'); private lastConnectionGeneration = 0; @@ -185,12 +261,109 @@ export class McpClientManager { tools: Object.freeze([]), }); + /** Secret values seen flowing through OAuth storage, per server. Config + * secrets are knowable synchronously; stored tokens are not — so every + * storage read/write harvests them for the outbound-message scrubber. + * Values survive record deletion on purpose: a late error can still + * reflect a token that was just dropped. */ + private readonly storedSecrets = new Map>(); + + /** Owns every credential state transition: per-server lanes, epochs and + * versioned CAS writes live in one place (credential-coordinator.ts). */ + private coordinator?: McpCredentialCoordinator; + constructor(options: McpClientManagerOptions = {}) { this.timeouts = { ...DEFAULT_TIMEOUTS, ...options.timeouts }; this.now = options.now ?? Date.now; this.clientName = options.clientName ?? 'maka'; this.clientVersion = options.clientVersion ?? '0.1.0'; this.excludedStdioEnvironmentKeys = [...(options.excludedStdioEnvironmentKeys ?? [])]; + this.oauthStorage = options.oauthStorage + ? this.harvestingStorage(options.oauthStorage) + : undefined; + this.coordinator = this.oauthStorage + ? new McpCredentialCoordinator(this.oauthStorage) + : undefined; + } + + private harvestingStorage(storage: McpOAuthStorage): McpOAuthStorage { + const harvest = ( + serverId: string, + record: { tokens?: unknown; clientInformation?: unknown; codeVerifier?: unknown } | undefined, + ) => { + if (!record) return; + const tokens = record.tokens as + | { access_token?: unknown; refresh_token?: unknown; id_token?: unknown } + | undefined; + const client = record.clientInformation as { client_secret?: unknown } | undefined; + const verifier = record.codeVerifier; + const values = [ + tokens?.access_token, + tokens?.refresh_token, + tokens?.id_token, + client?.client_secret, + // The PKCE verifier is a secret too: a hostile token endpoint can + // echo the code_verifier parameter it was sent back into an error. + verifier, + ]; + const secrets = this.storedSecrets.get(serverId) ?? new Set(); + for (const value of values) { + if (typeof value !== 'string' || value.length === 0) continue; + // Re-insertion moves the value to the tail: the set stays ordered + // by recency, so the eviction below drops the OLDEST harvested + // material once a long-lived process has rotated tokens enough + // times that scrubbing every historical value would grow without + // bound. Recently seen values are the ones a server can still + // reflect. + secrets.delete(value); + secrets.add(value); + } + while (secrets.size > MAX_HARVESTED_SECRETS_PER_SERVER) { + const oldest: string = secrets.values().next().value as string; + secrets.delete(oldest); + } + if (secrets.size > 0) this.storedSecrets.set(serverId, secrets); + }; + const underlyingCompareAndSet = storage.compareAndSet?.bind(storage); + return { + get: async (serverId) => { + const record = await storage.get(serverId); + harvest(serverId, record); + return record; + }, + set: async (serverId, record) => { + harvest(serverId, record); + await storage.set(serverId, record); + }, + delete: (serverId) => storage.delete(serverId), + // The coordinator downgrades to unconditional writes when the backend + // hides its CAS — forward it, or external-writer conflicts are never + // detected. + ...(underlyingCompareAndSet + ? { + compareAndSet: async ( + serverId: string, + expectedVersion: number | null, + record: McpOAuthRecord, + ) => { + harvest(serverId, record); + return underlyingCompareAndSet(serverId, expectedVersion, record); + }, + } + : {}), + }; + } + + /** Everything the scrubber must never let out for this server: the + * config's secret values plus any token material seen in storage. */ + private secretsFor(serverId: string, config: McpServerConfig): SecretInventory { + const inventory = collectConfigSecrets(config); + for (const value of this.storedSecrets.get(serverId) ?? []) { + (value.length >= MIN_SUBSTITUTION_LENGTH ? inventory.substitute : inventory.withhold).push( + value, + ); + } + return inventory; } onChange(listener: McpManagerChangeListener): () => void { @@ -208,16 +381,75 @@ export class McpClientManager { private async syncNow(config: McpConfigFile): Promise { const desired = new Set(Object.keys(config.mcpServers)); + // A failed erase must not abandon the rest of the reconciliation: the + // config file is already written, so stopping here would leave every + // OTHER added/changed server diverged until the next sync. The blocked + // server stays blocked; the failures reject the sync at the end. + const removalFailures: unknown[] = []; await Promise.all( [...this.connections.keys()] .filter((serverId) => !desired.has(serverId)) - .map((serverId) => this.disconnect(serverId, true)), + .map(async (serverId) => { + const entry = this.connections.get(serverId); + // Credentials first, connection second: a removed server's stored + // OAuth tokens are a hazard — a same-id server added back later + // must not inherit them. Erasing is the authoritative transition; + // only after it succeeds may connection ownership be released. On + // failure the entry stays, blocked — the next sync retries. + try { + await this.forgetAuthorization(serverId, entry?.credentialCleanupOwed ?? entry?.config); + } catch (error) { + if (entry) { + await this.blockForCredentialCleanup( + serverId, + entry, + entry.credentialCleanupOwed ?? entry.config, + error, + ); + } + removalFailures.push(error); + return; + } + await this.disconnect(serverId, true); + }), ); const connectIds: string[] = []; for (const [serverId, serverConfig] of Object.entries(config.mcpServers)) { const fingerprint = stableConfigFingerprint(serverConfig); const current = this.connections.get(serverId); - if (current && current.fingerprint !== fingerprint) await this.disconnect(serverId, true); + if (current && current.fingerprint !== fingerprint) { + // A changed endpoint URL invalidates the credentials: replaying the + // old server's bearer token against a new URL is exactly the leak to + // avoid. A headers-only edit keeps them. Credential retirement is a + // prerequisite: endpoint ownership changes ONLY after the erase + // succeeds — on failure the old entry stays, blocked and never + // connectable, and the new config is not adopted. The next sync + // retries the erase against the still-owed old config. + const owed = current.credentialCleanupOwed + ? current.credentialCleanupOwed + : remoteUrlChanged(current.config, serverConfig) + ? current.config + : undefined; + if (owed) { + try { + await this.forgetAuthorization(serverId, owed); + } catch (error) { + await this.blockForCredentialCleanup(serverId, current, owed, error); + // Same contract as the removal loop: the config is already + // written to the NEW endpoint while the old one stays blocked — + // the caller that wrote it needs the failure, not a clean + // resolve that hides the divergence. + removalFailures.push(error); + continue; + } + current.credentialCleanupOwed = undefined; + } + await this.disconnect(serverId, true); + } else if (current?.credentialCleanupOwed) { + // Same fingerprint again: the config reverted to (or never left) + // the endpoint the credentials belong to — nothing is owed. + current.credentialCleanupOwed = undefined; + } if (!this.connections.has(serverId)) { this.connections.set(serverId, { config: serverConfig, @@ -234,6 +466,34 @@ export class McpClientManager { if (serverConfig.enabled !== false) connectIds.push(serverId); } await Promise.all(connectIds.map((serverId) => this.connect(serverId).catch(() => {}))); + if (removalFailures.length > 0) throw removalFailures[0]; + } + + /** Fail-closed holding state for a server whose stored credentials could + * not be erased: the transport is torn down, the entry is kept under its + * OLD config (the one the credentials belong to), and connect() refuses + * it until a later sync retires the credentials. */ + private async blockForCredentialCleanup( + serverId: string, + entry: Connection, + owed: McpServerConfig, + error: unknown, + ): Promise { + entry.credentialCleanupOwed = owed; + await this.disconnect(serverId, false).catch(() => {}); + this.update(entry, { + ...entry.status, + state: 'error', + toolCount: 0, + tools: [], + // The inventory matters here too: a credential-store failure can echo + // record contents into its message. + error: `stored credentials for the previous endpoint could not be removed: ${errorMessage( + error, + this.secretsFor(serverId, owed), + )}`, + updatedAt: this.now(), + }); } statuses(): McpServerStatus[] { @@ -253,8 +513,21 @@ export class McpClientManager { if (this.closed) throw new Error('MCP client manager is closed'); const entry = this.requireConnection(serverId); if (entry.closing) throw new Error(`MCP server "${serverId}" is closing`); + if (entry.credentialCleanupOwed) { + // Fail closed: the previous endpoint's credentials still exist, so no + // connection may proceed until a sync retires them. + throw new Error( + `MCP server "${serverId}" is blocked: stored credentials for the previous endpoint could not be removed`, + ); + } if (entry.config.enabled === false) return cloneStatus(entry.status); if (entry.status.state === 'connected') return cloneStatus(entry.status); + if (this.interactiveRounds.has(serverId)) { + // Deferred, not failed: the round's finish (or the next sync after an + // abandon) reconnects. Starting a connect here would persist discovery + // state mid-round and rotate the record under the round's fence. + return cloneStatus(entry.status); + } if (entry.connectPromise) return entry.connectPromise; const controller = new AbortController(); entry.connectController = controller; @@ -495,6 +768,7 @@ export class McpClientManager { `unsafe integer argument at ${formatMcpDiagnosticText(headerArguments.path.join('.'))}`, ); } + const inventory = this.secretsFor(serverId, entry.config); const preparation = snapshot.callPreparation ?? (snapshot.callPreparation = this.toolCallPreparer.prepare(snapshot.definition)); @@ -514,7 +788,27 @@ export class McpClientManager { }, ); } catch (error) { - throw normalizeToolCallError(serverId, toolName, error, options.signal); + // A 401 after connect means the server revoked the session, not that + // one call hiccuped: leave `connected` and the UI never offers the + // login it now needs. + if ( + isAuthRequiredError(error) && + this.connections.get(serverId) === entry && + entry.client === client + ) { + this.markError(entry, error); + } + // The transport error can carry reflected request material (the body + // of a failed POST); scrub it like every other outbound message. The + // cause chain still holds the RAW transport error — the rejection + // leaves the manager (IPC, logs, telemetry), and any cause-aware + // serializer downstream would expose it — so the retained cause is an + // allowlisted copy: typed identity (name, code, status, SDK brands) + // with a scrubbed message and no deeper chain or payload fields. + const normalized = normalizeToolCallError(serverId, toolName, error, options.signal); + normalized.message = scrubKnownSecrets(normalized.message, inventory); + normalized.cause = sanitizedCause(normalized.cause, inventory); + throw normalized; } // The SDK's legacy compatibility schema defaults a missing content array // before returning, but retains deferred-result compatibility fields. @@ -536,7 +830,12 @@ export class McpClientManager { throw new McpToolCallError(serverId, toolName, 'server returned invalid content'); } if (result.isError) { - throw new McpToolCallError(serverId, toolName, summarizeErrorContent(result.content)); + // The server writes this text; it can echo a secret it was sent. + throw new McpToolCallError( + serverId, + toolName, + scrubKnownSecrets(redactSecrets(summarizeErrorContent(result.content)), inventory), + ); } const validateOutput = preparation.value.validateOutput; if (validateOutput) { @@ -557,9 +856,11 @@ export class McpClientManager { }); } } + // Success payloads cross toward the renderer and the transcript too; a + // server can embed the credential it was just sent into a result. return { - content: result.content.map(normalizeContent), - structuredContent: result.structuredContent, + content: deepScrub(result.content.map(normalizeContent), inventory), + structuredContent: deepScrub(result.structuredContent, inventory), }; } @@ -656,7 +957,10 @@ export class McpClientManager { entry.subscriptionDiagnostic = subscriptionFailure ? subscriptionFailure.reason === 'unhonored' ? formatSubscriptionDiagnostic(serverId, 'did not honor tool-list changes') - : formatSubscriptionDiagnostic(serverId, 'is unavailable', subscriptionFailure.cause) + : scrubKnownSecrets( + formatSubscriptionDiagnostic(serverId, 'is unavailable', subscriptionFailure.cause), + this.secretsFor(serverId, entry.config), + ) : undefined; // Initial discovery shares the generation-fenced, single-flight refresh // owner with explicit refreshes and live change signals. A notification @@ -700,6 +1004,10 @@ export class McpClientManager { if (initialRefresh.suppressed) { entry.refreshDiagnostic = errorMessage(this.toolRefreshFrequencyError(serverId)); } + const authenticated = + !isMcpStdioConfig(entry.config) && this.oauthStorage + ? Boolean((await this.oauthStorage.get(serverId))?.tokens) + : undefined; this.update(entry, { serverId, state: 'connected', @@ -709,6 +1017,7 @@ export class McpClientManager { tools: initialRefresh.descriptors, error: projectConnectionDiagnostics(entry), stderrTail: entry.status.stderrTail, + ...(authenticated !== undefined ? { authenticated } : {}), updatedAt: this.now(), }); if ( @@ -769,7 +1078,15 @@ export class McpClientManager { } else { this.markError(entry, exposedError); } - throw exposedError; + // The status above is scrubbed; the rejection leaves the manager too + // (reconnect → IPC → renderer) and must not carry the raw message or + // cause chain. Like the tool-call path, the cause survives as an + // allowlisted sanitized copy — a plain DNS/TLS/refused failure keeps + // its underlying explanation (including the per-transport aggregate). + const inventory = this.secretsFor(serverId, entry.config); + const rejection = scrubbedError(exposedError, inventory); + rejection.cause = sanitizedCause(exposedError.cause, inventory); + throw rejection; } } @@ -790,7 +1107,7 @@ export class McpClientManager { ), stderr: 'pipe', }); - attachStderrTail(transport, entry, () => { + attachStderrTail(transport, entry, collectConfigSecrets(entry.config), () => { if (this.connections.get(serverId) === entry) this.emit(entry.status); }); const { client, events } = this.createClient('legacy'); @@ -807,7 +1124,7 @@ export class McpClientManager { }; } catch (error) { await safeClose(client, transport); - throw enrichStdioError(error, entry.status.stderrTail); + throw enrichStdioError(error, entry.status.stderrTail, collectConfigSecrets(entry.config)); } } const remoteConfig: McpRemoteServerConfig = entry.config; @@ -816,12 +1133,40 @@ export class McpClientManager { if (requested === 'sse' && preference !== 'legacy') { throw new Error(`MCP legacy SSE transport does not support protocol ${preference}`); } + const authProvider = await this.backgroundAuthProvider(serverId, remoteConfig, signal); + const serverUrl = new URL(remoteConfig.url); + // One authority per header: when OAuth is in play for this server — + // a static client is configured, or a stored record exists — the OAuth + // bearer owns Authorization, and a configured Authorization header is + // excluded rather than left to fight (and win over) the fresh token. + // A plain static-bearer config with no OAuth involvement keeps its + // header untouched. The config store rejects the explicit conflict + // (oauth block + Authorization header) outright. + const record = this.coordinator ? await this.coordinator.read(serverId) : undefined; + // The raw record only counts when it is BOUND to this endpoint: after an + // offline mcp.json repoint the stale record's tokens will be dropped by + // the provider, so they must not strip a configured header either. + const boundRecord = record?.serverUrl === remoteConfig.url ? record : undefined; + const oauthOwnsAuthorization = + authProvider !== undefined && + (Boolean(remoteConfig.oauth) || + Boolean(boundRecord?.tokens || boundRecord?.clientInformation)); + const requestHeaders = oauthOwnsAuthorization + ? withoutAuthorizationHeader(remoteConfig.headers) + : remoteConfig.headers; + // The SDK reuses request headers for OAuth discovery, registration and + // token requests too, which would leak a configured resource secret (an + // Authorization or X-API-Key header) to a cross-origin authorization + // server. The scoped fetch confines them to the endpoint's own origin, + // hop by hop across redirects, and blocks cleartext downgrades. + const fetchImpl = scopedFetch(serverUrl, requestHeaders); let streamableFailure: unknown; if (requested !== 'sse') { - const evidence = createStreamableHandshakeEvidence(); + const evidence = createStreamableHandshakeEvidence(fetchImpl); const { client, events } = this.createClient(preference); - const transport = new StreamableHTTPClientTransport(new URL(remoteConfig.url), { - requestInit: { headers: remoteConfig.headers }, + const transport = new StreamableHTTPClientTransport(serverUrl, { + requestInit: { headers: requestHeaders }, + ...(authProvider ? { authProvider } : {}), fetch: evidence.fetch, }); const isClosed = this.watchClientClose(serverId, entry, client); @@ -848,8 +1193,10 @@ export class McpClientManager { } } const { client, events } = this.createClient('legacy'); - const transport = new SSEClientTransport(new URL(remoteConfig.url), { - requestInit: { headers: remoteConfig.headers }, + const transport = new SSEClientTransport(serverUrl, { + requestInit: { headers: requestHeaders }, + ...(authProvider ? { authProvider } : {}), + fetch: fetchImpl, }); const isClosed = this.watchClientClose(serverId, entry, client); try { @@ -867,6 +1214,413 @@ export class McpClientManager { } } + /** Provider for non-interactive connects: uses and refreshes stored + * tokens, but refuses to start a browser round. */ + private async backgroundAuthProvider( + serverId: string, + config: McpRemoteServerConfig, + signal?: AbortSignal, + ): Promise { + if (!this.oauthStorage) return undefined; + return new McpOAuthProvider({ + serverId, + serverUrl: config.url, + // The abort signal fences writes only while the CONNECT is in flight + // — connect() clears its controller in `finally`, so nothing aborts + // this flow once the connection is up. What fences a live + // connection's late writes is the epoch bump (logout/erase) and the + // generation/version beginFlow pins before the first network await: + // a cross-process logout during the handshake trips those, not the + // signal. + storage: await this.beginFlow(serverId, signal), + config: config.oauth, + clientName: this.clientName, + clientVersion: this.clientVersion, + }); + } + + private flowStorage(serverId: string, signal?: AbortSignal): McpOAuthStorage { + return this.requireCoordinator().flowStorage(serverId, signal ? { signal } : {}); + } + + /** Flow view with generation/version pinned before any remote await. */ + private beginFlow(serverId: string, signal?: AbortSignal): Promise { + return this.requireCoordinator().beginFlow(serverId, signal ? { signal } : {}); + } + + private requireCoordinator(): McpCredentialCoordinator { + if (!this.coordinator) throw new Error('MCP OAuth storage is not configured'); + return this.coordinator; + } + + private requireRemoteEntry(serverId: string): { + entry: Connection; + config: McpRemoteServerConfig; + } { + const entry = this.requireConnection(serverId); + if (isMcpStdioConfig(entry.config)) { + throw new Error( + `MCP server "${serverId}" is a stdio server; OAuth applies to remote servers`, + ); + } + if (entry.credentialCleanupOwed) { + // The same fail-closed gate connect() applies: no OAuth path — start, + // finish, or clear — may write fresh credential state while the + // previous endpoint's credentials are still owed retirement. The next + // sync retries the erase and unblocks the entry. + throw new Error( + `MCP server "${serverId}" is blocked: stored credentials for the previous endpoint could not be removed`, + ); + } + return { entry, config: entry.config }; + } + + private requireOAuthStorage(): McpOAuthStorage { + if (!this.oauthStorage) throw new Error('MCP OAuth storage is not configured'); + return this.oauthStorage; + } + + /** + * Begins an interactive authorization round. Runs discovery (and, when + * stored or configured credentials already satisfy the server, the silent + * token path); returns either `authorized` — nothing to open — or the + * authorization URL to open in the user's browser. The PKCE verifier and + * redirect URL are persisted, so the matching finishAuthorization call is + * process-restart-safe. + */ + async startAuthorization( + serverId: string, + redirectUrl: string, + options: { state?: string; signal?: AbortSignal } = {}, + ): Promise { + const { config } = this.requireRemoteEntry(serverId); + // The round is bound to a verifiable state from the first byte: minted + // here when the caller supplies none, so the pending record ALWAYS + // carries one and finishAuthorization always has something to verify. + const state = options.state ?? randomBytes(16).toString('hex'); + // Claim the round BEFORE any await, and retire any in-flight background + // connect: it writes discovery state through the same record and would + // trip this round's version fence (see interactiveRounds). + this.interactiveRounds.add(serverId); + try { + this.cancelConnect(serverId); + await this.connections.get(serverId)?.connectPromise?.catch(() => {}); + } catch (error) { + this.interactiveRounds.delete(serverId); + throw error; + } + return this.startAuthorizationRound(serverId, config, redirectUrl, state, options).catch( + (error) => { + this.interactiveRounds.delete(serverId); + throw error; + }, + ); + } + + private async startAuthorizationRound( + serverId: string, + config: McpRemoteServerConfig, + redirectUrl: string, + state: string, + options: { state?: string; signal?: AbortSignal }, + ): Promise { + // The caller's round deadline fences this flow's storage writes: a + // round abandoned by its owner must not land verifier/state/tokens + // over a newer round's record. beginFlow pins the persisted + // generation/version BEFORE the remote probe below — a logout in + // another process during that await must fence this flow, not hand it + // the tombstone as a fresh starting point. + const storage = await this.beginFlow(serverId, options.signal); + let authorizationUrl: URL | undefined; + const provider = new McpOAuthProvider({ + serverId, + serverUrl: config.url, + storage, + config: config.oauth, + clientName: this.clientName, + clientVersion: this.clientVersion, + interactive: { + redirectUrl, + state, + onAuthorizationUrl: (url) => { + authorizationUrl = url; + }, + }, + }); + // The interactive flow IS OAuth: it owns Authorization for every request + // it makes (discovery, registration, the probe, token exchange) — the + // same ownership rule openClient applies, unconditional here. A + // configured static header must not ride along and shadow the round's + // own bearer. + const fetchFn = scopedFetch( + new URL(config.url), + withoutAuthorizationHeader(config.headers), + options.signal, + ); + // A fresh 401 challenge may carry a scope (and resource_metadata URL) + // the authorization request must echo; the persisted discovery state + // covers the metadata but not the scope, so ask the endpoint directly. + const challenge = await probeAuthChallenge(config.url, fetchFn); + let result: Awaited>; + try { + result = await auth(provider, { + serverUrl: config.url, + scope: challenge?.scope || config.oauth?.scopes?.join(' ') || undefined, + ...(challenge?.resourceMetadataUrl + ? { resourceMetadataUrl: challenge.resourceMetadataUrl } + : {}), + fetchFn, + }); + } catch (error) { + throw scrubbedError(error, this.secretsFor(serverId, config)); + } + if (result === 'AUTHORIZED') { + this.interactiveRounds.delete(serverId); + await this.reconnect(serverId).catch(() => {}); + return { status: 'authorized' }; + } + if (!authorizationUrl) { + throw new Error(`MCP server "${serverId}" did not produce an authorization URL`); + } + // The authorization URL comes from remote metadata: the same provenance + // rule as every other remotely supplied destination applies before it + // is handed to a browser. (https, or loopback-http only when the user + // configured the server itself on loopback.) + try { + assertTransportSecurity(authorizationUrl, urlProvenance(new URL(config.url))); + } catch (error) { + // auth() already persisted this round's verifier and pending fields; + // a refused URL means the round can never complete. Clear them so + // pendingAuthorization stops advertising a dead round for the desktop + // resume path to rebind a listener to. + await provider.invalidateCredentials('verifier').catch(() => {}); + throw error; + } + // Both values are the SERVER'S claims — the issuer from the discovery + // this round performed, the scope from its challenge — surfaced so a + // consumer can show the user what they are consenting to BEFORE a + // browser opens, instead of burying the issuer inside the URL. + const discovery = await provider.discoveryState().catch(() => undefined); + const resolvedScope = challenge?.scope || config.oauth?.scopes?.join(' ') || undefined; + return { + status: 'redirect', + authorizationUrl: authorizationUrl.toString(), + state, + issuer: discovery?.authorizationServerUrl + ? `${discovery.authorizationServerUrl}` + : new URL(config.url).origin, + scopes: resolvedScope ? resolvedScope.split(' ').filter(Boolean) : [], + }; + } + + /** Exchanges the callback's authorization code, then reconnects. The + * callback payload is passed whole — including the RFC 9207 `iss` + * parameter, which the SDK validates against the discovered issuer to + * defeat authorization-server mix-ups; truncating it here would disable + * that check. */ + async finishAuthorization( + serverId: string, + callback: { code: string; iss?: string; state?: string }, + options: { signal?: AbortSignal } = {}, + ): Promise { + try { + return await this.finishAuthorizationRound(serverId, callback, options); + } finally { + // Success released it before its reconnect; every failure path + // releases here so an aborted exchange cannot park the connect gate. + this.interactiveRounds.delete(serverId); + } + } + + private async finishAuthorizationRound( + serverId: string, + callback: { code: string; iss?: string; state?: string }, + options: { signal?: AbortSignal } = {}, + ): Promise { + const authorizationCode = callback.code; + const { config } = this.requireRemoteEntry(serverId); + // The immediate read doubles as the flow's generation/version pin. + const storage = this.flowStorage(serverId, options.signal); + const record = await storage.get(serverId); + const pendingRedirectUrl = record?.pendingRedirectUrl; + if (!pendingRedirectUrl) { + throw new Error(`No pending authorization for MCP server "${serverId}"`); + } + // Two interactive rounds share one server's pending slot: the newer + // round's verifier overwrote the older one's. A caller that presents + // its round state must match the PERSISTED round, or its code would be + // exchanged against the wrong PKCE verifier. + // The binding fires whenever the PERSISTED round has a state — which + // startAuthorization now guarantees. Keying off the caller-supplied + // value instead would let a callback that simply omits `state` skip the + // check and redeem a foreign code against the pending verifier. + if (record?.pendingState !== undefined && record.pendingState !== callback.state) { + throw new Error( + `Authorization round for MCP server "${serverId}" was superseded by a newer login`, + ); + } + // The pending round is bound to the URL it was started against — the + // binding is REQUIRED, not merely checked when present: a pending + // record that cannot prove its endpoint must not mint tokens for one. + if (record?.pendingServerUrl !== config.url) { + throw new Error(`MCP server "${serverId}" changed its URL during authorization`); + } + const provider = new McpOAuthProvider({ + serverId, + serverUrl: config.url, + storage, + config: config.oauth, + clientName: this.clientName, + clientVersion: this.clientVersion, + interactive: { + redirectUrl: pendingRedirectUrl, + onAuthorizationUrl: () => { + throw new Error(`MCP server "${serverId}" restarted authorization during token exchange`); + }, + }, + }); + // Same ownership rule as startAuthorization: the round owns + // Authorization; the configured static header stays out of the exchange. + const fetchFn = scopedFetch( + new URL(config.url), + withoutAuthorizationHeader(config.headers), + options.signal, + ); + let result: Awaited>; + try { + result = await auth(provider, { + serverUrl: config.url, + authorizationCode, + ...(callback.iss !== undefined ? { iss: callback.iss } : {}), + fetchFn, + }); + } catch (error) { + // A token endpoint can reflect what it was sent (a static clientSecret, + // a header value, a token — or this round's authorization code) into + // error_description; none of it may reach the renderer through the + // flattened IPC error. + const inventory = this.secretsFor(serverId, config); + (authorizationCode.length >= MIN_SUBSTITUTION_LENGTH + ? inventory.substitute + : inventory.withhold + ).push(authorizationCode); + throw scrubbedError(error, inventory); + } + if (result !== 'AUTHORIZED') { + throw new Error(`Authorization for MCP server "${serverId}" did not complete`); + } + // Round over: release before reconnect, which the round gate would + // otherwise defer. + this.interactiveRounds.delete(serverId); + return this.reconnect(serverId); + } + + /** The persisted-but-unfinished interactive round, if any — enough for + * the desktop to rebind its loopback callback listener after a restart. */ + async pendingAuthorization( + serverId: string, + ): Promise<{ redirectUrl: string; state?: string } | undefined> { + if (!this.oauthStorage) return undefined; + const record = await this.oauthStorage.get(serverId); + if (!record?.codeVerifier || !record.pendingRedirectUrl) return undefined; + // Callable before the first sync populates connections — the boot-time + // listener rebind must not wait for slow connects. When the entry is + // known, its URL still gates the pending round; either way + // finishAuthorization re-checks pendingServerUrl before the exchange. + const entry = this.connections.get(serverId); + if (entry && isMcpStdioConfig(entry.config)) return undefined; + if ( + entry && + !isMcpStdioConfig(entry.config) && + record.pendingServerUrl && + record.pendingServerUrl !== entry.config.url + ) { + return undefined; + } + return { redirectUrl: record.pendingRedirectUrl, state: record.pendingState }; + } + + /** Abandons a persisted-but-dead interactive round: clears the verifier + * and pending fields, keeping tokens, client registration and discovery + * intact. The desktop controller calls this on TERMINAL login failures + * (denial, timeout, browser-launch failure) — otherwise the boot resume + * would rebind a listener for a round that can never complete and hold + * the per-server login guard on every restart. */ + async abandonAuthorization(serverId: string): Promise { + this.interactiveRounds.delete(serverId); + if (!this.coordinator) return; + const record = await this.coordinator.read(serverId); + if (!record?.codeVerifier && !record?.pendingRedirectUrl && !record?.pendingState) return; + const observedVersion = record.version ?? 0; + const storage = this.flowStorage(serverId); + // The coordinator's flow view always supplies update(); a silent no-op + // else-branch on a security-relevant clear must not exist. + if (!storage.update) throw new Error('MCP credential coordinator view lost update()'); + { + try { + await storage.update(serverId, (basis) => { + // Pinned to the round we DECIDED to abandon: a fresh flow view + // captures whatever version it reads at write time, so without + // this check an abandon racing a NEWER login round would happily + // CAS away that round's verifier and state. + if ((basis.version ?? 0) !== observedVersion) throw new McpAbandonSupersededError(); + const next = { ...basis }; + delete next.codeVerifier; + delete next.pendingRedirectUrl; + delete next.pendingServerUrl; + delete next.pendingState; + return next; + }); + } catch (error) { + // A superseded abandon is a no-op, not a failure: the round we + // wanted dead no longer exists. + if (!(error instanceof McpAbandonSupersededError)) throw error; + } + } + } + + /** Public variant for the IPC layer: drop a server's credentials BEFORE + * its config is removed, so a delete failure aborts the removal while + * everything is still recoverable — instead of leaving an orphaned token + * a same-id re-add would inherit. */ + async forgetServerCredentials(serverId: string): Promise { + await this.forgetAuthorization(serverId, this.connections.get(serverId)?.config); + } + + /** Drops any stored OAuth record for a server that is being removed or + * whose endpoint changed. A failure propagates — the callers must not + * proceed as if the credentials were gone. Deliberately NOT gated on the + * server's current kind: an offline edit can convert a credentialed + * remote server to stdio, and skipping the erase for stdio would let the + * stale record survive the id being freed for reuse. A stdio server with + * NO stored record skips the erase entirely: writing a tombstone there + * would grow the credential file by one junk record per removed stdio + * server, guarding a resurrection no flow can attempt (flows only exist + * for remote configs, and the remote-conversion case reads a record). */ + private async forgetAuthorization( + serverId: string, + config?: McpServerConfig, + options: { signal?: AbortSignal } = {}, + ): Promise { + if (!this.coordinator) return; + if (config && isMcpStdioConfig(config) && !(await this.coordinator.read(serverId))) return; + await this.coordinator.erase(serverId, options); + } + + /** Drops stored tokens and registration, returning the server to needs-auth. */ + async clearAuthorization( + serverId: string, + options: { signal?: AbortSignal } = {}, + ): Promise { + const { config } = this.requireRemoteEntry(serverId); + this.interactiveRounds.delete(serverId); + await this.forgetAuthorization(serverId, config, options); + await this.reconnect(serverId).catch(() => {}); + const status = this.status(serverId); + if (!status) throw new Error(`Unknown MCP server: ${serverId}`); + return status; + } + private createClient(preference: McpProtocolPreference): { client: Client; events: McpClientEventBridge; @@ -934,14 +1688,14 @@ export class McpClientManager { if (!this.isCurrentClient(serverId, entry, client, connectionGeneration)) return; if (error) { const exposed = safeMcpOperationError(serverId, 'tool-list change signal failed', error); - entry.refreshDiagnostic = errorMessage(exposed); + entry.refreshDiagnostic = errorMessage(exposed, this.secretsFor(serverId, entry.config)); this.publishConnectionDiagnostics(entry); return; } void this.refreshToolsAfterNotification(serverId, entry, client, connectionGeneration).catch( (failure) => { if (!this.isCurrentClient(serverId, entry, client, connectionGeneration)) return; - const diagnostic = errorMessage(failure); + const diagnostic = errorMessage(failure, this.secretsFor(serverId, entry.config)); if (entry.refreshDiagnostic === diagnostic) return; entry.refreshDiagnostic = diagnostic; this.publishConnectionDiagnostics(entry); @@ -1057,7 +1811,10 @@ export class McpClientManager { failure = error; } if (!this.ownsToolRefresh(serverId, entry, state)) { - throw safeMcpOperationError(serverId, 'connection changed during tool refresh', failure); + throw scrubbedError( + safeMcpOperationError(serverId, 'connection changed during tool refresh', failure), + this.secretsFor(serverId, entry.config), + ); } const notificationState = entry.refreshNotificationState; const notificationStateMatches = @@ -1067,17 +1824,23 @@ export class McpClientManager { if (failure !== undefined) { if (refreshSuppressed) throw this.toolRefreshFrequencyError(serverId); const exposed = safeMcpOperationError(serverId, 'tool refresh failed', failure); - entry.refreshDiagnostic = errorMessage(exposed); + entry.refreshDiagnostic = errorMessage(exposed, this.secretsFor(serverId, entry.config)); this.publishConnectionDiagnostics(entry); if (!this.ownsToolRefresh(serverId, entry, state)) { - throw safeMcpOperationError( - serverId, - 'connection changed during tool refresh', - failure, + // The raw failure text (and its cause chain) can reflect request + // material; this rejection leaves the manager like every other. + throw scrubbedError( + safeMcpOperationError(serverId, 'connection changed during tool refresh', failure), + this.secretsFor(serverId, entry.config), ); } if (state.pending) continue; - throw exposed; + // A 401 through the PUBLIC refresh path is the same authorization + // loss as one through the notification path: the server must + // leave `connected` and its snapshot must stop being callable + // before the caller sees the error. + if (!state.initial && isAuthRequiredError(failure)) this.markError(entry, failure); + throw scrubbedError(exposed, this.secretsFor(serverId, entry.config)); } if (!definitions) { if (refreshSuppressed) throw this.toolRefreshFrequencyError(serverId); @@ -1085,6 +1848,9 @@ export class McpClientManager { throw new Error(`MCP server "${serverId}" returned no tool definitions`); } + // Descriptions and schemas are server-authored and persist into the + // status/capability surfaces — scrub them like any outbound text. + const inventory = this.secretsFor(serverId, entry.config); const snapshot = createToolSnapshot( serverId, definitions, @@ -1092,6 +1858,7 @@ export class McpClientManager { this.bindingManagerId, state.connectionGeneration, entry.toolSnapshot, + (descriptor) => deepScrub(descriptor, inventory), ); latestSnapshot = snapshot; if (notificationStateMatches) notificationState.lastPassCompletedAt = this.now(); @@ -1185,9 +1952,39 @@ export class McpClientManager { } private markError(entry: Connection, error: unknown): void { + // Leaving `connected` invalidates the published tool bindings; dropping + // the snapshot revises the callable set so stale capabilities cannot + // survive in the Runtime Host. + if (entry.status.state === 'connected' && entry.toolSnapshot.size > 0) { + this.replaceToolSnapshot(entry, new Map()); + } + // One lifecycle owner per connection generation: the failed generation + // retires HERE. Keeping the old client on the entry would let a later + // connect() overwrite these fields on success without closing it, + // leaking its transport and its server-side session. + const retiredClient = entry.client; + const retiredTransport = entry.transport; + entry.client = undefined; + entry.transport = undefined; + entry.stdioTransport = undefined; + entry.connectionGeneration = undefined; + entry.refreshState = undefined; + entry.refreshNotificationState = undefined; + if (retiredClient) void safeClose(retiredClient, retiredTransport).catch(() => {}); + if (isAuthRequiredError(error)) { + this.update(entry, { + ...entry.status, + state: 'needs-auth', + toolCount: 0, + tools: [], + error: undefined, + updatedAt: this.now(), + }); + return; + } this.update(entry, { ...this.makeStatus(entry.status.serverId, 'error'), - error: errorMessage(error), + error: errorMessage(error, this.secretsFor(entry.status.serverId, entry.config)), stderrTail: entry.status.stderrTail, }); } @@ -1347,7 +2144,7 @@ function shouldFallbackToLegacySse(options: { ); } -function createStreamableHandshakeEvidence(): { +function createStreamableHandshakeEvidence(base: FetchLike = globalThis.fetch): { fetch: FetchLike; hasAcceptedInitialize(): boolean; } { @@ -1361,7 +2158,7 @@ function createStreamableHandshakeEvidence(): { if (readJsonRpcMethods(init?.body).includes('notifications/initialized')) { acceptedInitialize = true; } - return globalThis.fetch(url, init); + return base(url, init); }, hasAcceptedInitialize: () => acceptedInitialize, }; @@ -1410,6 +2207,8 @@ function createToolSnapshot( managerId: string, connectionGeneration: number, previousEntries?: ReadonlyMap, + scrubDescriptor: (descriptor: McpToolDescriptor) => McpToolDescriptor = (descriptor) => + descriptor, ): { entries: Map; descriptors: McpToolDescriptor[]; @@ -1435,7 +2234,7 @@ function createToolSnapshot( throw new Error(`MCP server "${serverId}" lost Tool "${tool.name}" during validation`); } const definition = discovered.definition; - const descriptor = descriptorFromTool(serverId, definition); + const descriptor = scrubDescriptor(descriptorFromTool(serverId, definition)); const definitionFingerprint = discovered.definitionFingerprint; const previous = previousEntries?.get(definition.name); const binding = @@ -1577,6 +2376,7 @@ export function buildStdioEnvironment( function attachStderrTail( transport: StdioClientTransport, entry: Connection, + secrets: SecretInventory, onUpdate: () => void, ): void { let pending = ''; @@ -1585,7 +2385,7 @@ function attachStderrTail( let physicalSuffix = ''; const append = (lines: string[]) => { const rendered = lines - .map((line) => formatMcpDiagnosticText(line, STDERR_LINE_CHARS)) + .map((line) => formatMcpDiagnosticText(scrubKnownSecrets(line, secrets), STDERR_LINE_CHARS)) .filter(Boolean); if (rendered.length === 0) return; const next = [...(entry.status.stderrTail ?? []), ...rendered] @@ -1660,9 +2460,13 @@ function attachStderrTail( stream?.once('close', flush); } -function enrichStdioError(error: unknown, stderrTail?: string[]): Error { +function enrichStdioError( + error: unknown, + stderrTail: string[] | undefined, + secrets: SecretInventory = EMPTY_INVENTORY, +): Error { const suffix = stderrTail?.length ? `\nstderr:\n${stderrTail.join('\n')}` : ''; - return new Error(`${errorMessage(error)}${suffix}`, { cause: error }); + return new Error(`${errorMessage(error, secrets)}${suffix}`, { cause: error }); } async function safeClose( @@ -1704,6 +2508,16 @@ async function connectRemoteCandidate( } } +/** True when a reconfigured server no longer talks to the endpoint its + * stored credentials were issued for: the URL changed, or the entry + * switched between stdio and remote. A headers-only edit returns false. */ +function remoteUrlChanged(previous: McpServerConfig, next: McpServerConfig): boolean { + const previousStdio = isMcpStdioConfig(previous); + const nextStdio = isMcpStdioConfig(next); + if (previousStdio || nextStdio) return previousStdio !== nextStdio; + return previous.url !== next.url; +} + function stableConfigFingerprint(config: McpServerConfig): string { return JSON.stringify(sortValue(config)); } @@ -1745,11 +2559,248 @@ function deepFreeze(value: T): T { return Object.freeze(value); } -function errorMessage(error: unknown): string { - return formatMcpDiagnosticText( - error instanceof Error ? error.message : String(error), - MCP_ERROR_DIAGNOSTIC_CODE_POINTS, - ); +type SecretInventory = McpSecretInventory; + +const EMPTY_INVENTORY = EMPTY_MCP_SECRET_INVENTORY; +const MIN_SUBSTITUTION_LENGTH = MCP_SECRET_MIN_SUBSTITUTION_LENGTH; + +function errorMessage(error: unknown, secrets: SecretInventory = EMPTY_INVENTORY): string { + // Scrub before the diagnostic formatter truncates: a secret straddling + // the truncation boundary would otherwise leave a matching prefix behind. + const raw = error instanceof Error ? error.message : String(error); + return formatMcpDiagnosticText(scrubKnownSecrets(raw, secrets), MCP_ERROR_DIAGNOSTIC_CODE_POINTS); +} + +/** The shared secret-location plan (@maka/core/mcp-secrets) is the single + * authority for which config positions carry credentials; this derives the + * scrub inventory from it. */ +function collectConfigSecrets(config: McpServerConfig): SecretInventory { + return mcpSecretInventory(config); +} + +function scrubKnownSecrets(message: string, secrets: SecretInventory): string { + return scrubKnownMcpSecrets(message, secrets); +} + +/** Rebuilds an outbound error with its message scrubbed of the given secret + * values. The cause chain is dropped deliberately: causes carry the raw + * upstream messages this exists to contain. */ +function scrubbedError(error: unknown, secrets: SecretInventory): Error { + const scrubbed = new Error(errorMessage(error, secrets)); + if (error instanceof Error) { + scrubbed.name = error.name; + const code = (error as { code?: unknown }).code; + if (typeof code === 'number') (scrubbed as { code?: number }).code = code; + } + // The cause chain is dropped, but the auth signal it carried must survive + // the scrub — the notification handler and every caller key off it to + // turn a 401 into needs-auth instead of a dead-end error. + if (isAuthRequiredError(error) && !isAuthRequiredError(scrubbed)) { + scrubbed.name = 'UnauthorizedError'; + } + return scrubbed; +} + +function deepScrub(value: T, secrets: SecretInventory): T { + return deepScrubMcpSecrets(value, secrets); +} + +/** Asks the endpoint for its current WWW-Authenticate challenge with an + * unauthenticated request. GET first (the SSE stream), then — because a + * Streamable HTTP server may answer GET with 405 and only challenge the + * initialize POST — an initialize POST. Best-effort: a server that answers + * 401 to neither simply yields no challenge context. */ +async function probeAuthChallenge( + serverUrl: string, + fetchImpl: typeof fetch, +): Promise<{ scope?: string; resourceMetadataUrl?: URL } | undefined> { + const attempts: RequestInit[] = [ + { method: 'GET', headers: { accept: 'text/event-stream, application/json' } }, + { + method: 'POST', + headers: { + 'content-type': 'application/json', + accept: 'application/json, text/event-stream', + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 0, + method: 'initialize', + params: { + // The SDK's current version, not a pinned literal: a server that + // only accepts the current protocol would 400 an outdated probe + // and the challenge (with its scope) would never be seen. + protocolVersion: LATEST_PROTOCOL_VERSION, + capabilities: {}, + clientInfo: { name: 'maka-challenge-probe', version: '0' }, + }, + }), + }, + ]; + for (const init of attempts) { + const challenge = await probeOnce(serverUrl, fetchImpl, init); + // A 401 whose WWW-Authenticate carried no usable parameters is not an + // answer — the POST attempt may still surface scope/metadata. + if (challenge && (challenge.scope || challenge.resourceMetadataUrl)) return challenge; + } + return undefined; +} + +async function probeOnce( + serverUrl: string, + fetchImpl: typeof fetch, + init: RequestInit, +): Promise<{ scope?: string; resourceMetadataUrl?: URL } | undefined> { + try { + const response = await fetchImpl(serverUrl, init); + await response.body?.cancel().catch(() => {}); + if (response.status !== 401) { + // If the unauthenticated initialize actually succeeded, the server + // may have opened a session for the probe; close it politely. + const sessionId = response.headers.get('mcp-session-id'); + if (sessionId) { + await fetchImpl(serverUrl, { + method: 'DELETE', + headers: { 'mcp-session-id': sessionId }, + }) + .then((cleanup) => cleanup.body?.cancel()) + .catch(() => {}); + } + return undefined; + } + return extractWWWAuthenticateParams(response); + } catch { + return undefined; + } +} + +const MAX_FETCH_REDIRECTS = 5; +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); +/** Credential-bearing headers a cross-origin redirect must shed — the rule + * undici applies to Authorization on its own, re-created here because the + * manual redirect loop takes over hop handling (and the SDK's authProvider + * injects Authorization after config headers are scoped). */ +// The MCP session identifier is a credential too: an established Streamable +// HTTP transport sends it on every request, and a 307/308 to another origin +// would hand the live session to that origin. `last-event-id` rides along — +// it exposes resumable stream position for that session. +const CREDENTIAL_HEADERS = [ + 'authorization', + 'cookie', + 'proxy-authorization', + 'mcp-session-id', + 'last-event-id', +]; + +/** Every URL this client will actually talk to — the endpoint, its + * redirects, and the OAuth discovery/registration/token endpoints the SDK + * routes through the same fetch — must not carry credentials over + * cleartext off the machine. Mirrors the config store's rule for the + * endpoint URL itself. */ + +/** Wraps fetch so credentials stay scoped hop by hop: configured resource + * headers ride only requests to the MCP endpoint's own origin, any redirect + * that leaves an origin sheds Authorization/Cookie for the rest of the + * chain, and no hop may downgrade to non-loopback cleartext http. Redirects + * are followed manually because undici forwards custom headers (an + * X-API-Key) across a cross-origin 307, and the SDK routes OAuth discovery / + * registration / token calls through this same fetch. */ +function scopedFetch( + serverUrl: URL, + headers: Record | undefined, + signal?: AbortSignal, +): typeof fetch { + const configured = headers ?? {}; + const guardedKeys = Object.keys(configured).map((key) => key.toLowerCase()); + const provenance: UrlProvenance = urlProvenance(serverUrl); + const initFor = ( + target: URL, + init: RequestInit | undefined, + credentialsShed: boolean, + ): RequestInit => { + const merged = new Headers(init?.headers); + if (target.origin === serverUrl.origin) { + for (const [key, value] of Object.entries(configured)) { + if (!merged.has(key)) merged.set(key, value); + } + } else { + for (const key of guardedKeys) merged.delete(key); + } + if (credentialsShed) { + for (const key of CREDENTIAL_HEADERS) merged.delete(key); + } + return { + ...init, + headers: merged, + redirect: 'manual', + // The round's deadline aborts in-flight requests too, not only the + // caller's await: a hung endpoint must not keep the flow alive. + ...(signal ? { signal: init?.signal ? AbortSignal.any([init.signal, signal]) : signal } : {}), + }; + }; + return (async (input: RequestInfo | URL, init?: RequestInit) => { + // The SDK's transports and auth flows always call (url, init); a + // preassembled Request would hide its headers and body from the + // per-hop policy, so refuse rather than guess. + if (typeof input !== 'string' && !(input instanceof URL)) { + throw new Error('MCP scoped fetch requires a URL input'); + } + let target = new URL(input); + let request = init; + // Once any hop crosses an origin, credential headers stay off for the + // remainder of the chain — even if it bounces back. + let credentialsShed = false; + for (let hop = 0; hop <= MAX_FETCH_REDIRECTS; hop += 1) { + assertTransportSecurity(target, provenance); + const response = await fetch(target, initFor(target, request, credentialsShed)); + if (!REDIRECT_STATUSES.has(response.status)) return response; + const location = response.headers.get('location'); + if (!location) return response; + await response.body?.cancel().catch(() => {}); + const method = (request?.method ?? 'GET').toUpperCase(); + if ( + response.status === 303 || + ((response.status === 301 || response.status === 302) && method === 'POST') + ) { + request = { ...request, method: 'GET', body: undefined }; + } else if ( + request?.body !== undefined && + request.body !== null && + typeof request.body !== 'string' + ) { + // A stream body cannot be replayed for the next hop. + throw new Error(`MCP request to ${target.origin} was redirected and cannot be resent`); + } + const next = new URL(location, target); + if (next.origin !== target.origin) credentialsShed = true; + target = next; + } + throw new Error(`MCP request exceeded ${MAX_FETCH_REDIRECTS} redirects`); + }) as typeof fetch; +} + +/** True when the failure means "the server wants the user to log in": + * our own refusal to open a browser mid-connect, the SDK's + * UnauthorizedError, or a raw 401 from either remote transport (matched by + * name — the error may cross package boundaries where instanceof breaks). + * Walks the cause chain. */ +function isAuthRequiredError(error: unknown): boolean { + for (let current = error; current instanceof Error; current = current.cause as Error) { + if (current instanceof McpAuthRequiredError) return true; + if (current.name === 'UnauthorizedError' || current.name === 'McpAuthRequiredError') + return true; + const code = (current as { code?: unknown }).code; + const status = (current as { status?: unknown }).status; + if ( + (code === 401 || status === 401) && + (current.name === 'StreamableHTTPError' || + current.name === 'SseError' || + current.name === 'SdkHttpError') + ) { + return true; + } + } + return false; } function safeMcpOperationError( @@ -1763,10 +2814,63 @@ function safeMcpOperationError( return new Error(formatMcpDiagnosticText(message, MCP_ERROR_DIAGNOSTIC_CODE_POINTS), { cause }); } +function withoutAuthorizationHeader( + headers: Record | undefined, +): Record | undefined { + if (!headers) return undefined; + const filtered = Object.fromEntries( + Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'authorization'), + ); + return filtered; +} + function stringValue(value: unknown): string | undefined { return typeof value === 'string' ? value : undefined; } +/** Rebuilds an outbound error cause with only its typed identity: name, + * numeric/string code, HTTP status, and the SDK's brand symbols (what + * `SdkError.isInstance` keys on) survive; the message is scrubbed and the + * deeper cause chain and any payload fields (response bodies, request data) + * are dropped — those are where an endpoint's reflection of credential + * material hides. */ +function sanitizedCause(error: unknown, secrets: SecretInventory): Error | undefined { + if (!(error instanceof Error)) return undefined; + if (error instanceof AggregateError) { + // A connect failure aggregates one error per attempted transport; + // keeping the shape (with each member sanitized) preserves the + // diagnosability the aggregate exists for. + const clean = new AggregateError( + error.errors.map((item) => sanitizedCause(item, secrets)).filter(Boolean) as Error[], + scrubKnownSecrets(error.message, secrets), + ); + clean.name = error.name; + return clean; + } + const clean = new Error(scrubKnownSecrets(error.message, secrets)); + clean.name = error.name; + const code = (error as { code?: unknown }).code; + if (typeof code === 'number' || typeof code === 'string') { + (clean as { code?: unknown }).code = code; + } + const status = (error as { status?: unknown }).status; + if (typeof status === 'number') (clean as { status?: unknown }).status = status; + for (const symbol of Object.getOwnPropertySymbols(error)) { + const descriptor = Object.getOwnPropertyDescriptor(error, symbol); + if (descriptor) Object.defineProperty(clean, symbol, descriptor); + } + return clean; +} + +/** Internal sentinel: the pending round this abandon targeted was replaced + * by a newer one before the clearing write landed. */ +class McpAbandonSupersededError extends Error { + constructor() { + super('MCP authorization round was superseded before it could be abandoned'); + this.name = 'McpAbandonSupersededError'; + } +} + function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } diff --git a/packages/mcp/src/oauth.ts b/packages/mcp/src/oauth.ts new file mode 100644 index 0000000000..3196f805e1 --- /dev/null +++ b/packages/mcp/src/oauth.ts @@ -0,0 +1,352 @@ +// packages/mcp/src/oauth.ts +// +// OAuth support for remote MCP servers, following the MCP authorization +// model (OAuth 2.1 + RFC 9728 protected-resource discovery, PKCE always). +// +// Split of responsibilities: +// - The SDK owns the protocol: discovery, dynamic registration, PKCE, +// token exchange and refresh all live in `client/auth.js`. +// - This module owns persistence and the interactive boundary. Tokens, +// registered-client records and in-flight PKCE verifiers go through an +// injected `McpOAuthStorage` (the desktop app backs it with the shared +// CredentialStore; tests use memory). Interaction is a mode switch: +// a background connect must never open a browser, so its provider +// REFUSES the redirect (surfacing `needs-auth`), while a user-initiated +// login CAPTURES the authorization URL for the caller to open. + +import type { + OAuthClientMetadata, + OAuthClientProvider, + OAuthDiscoveryState, + StoredOAuthClientInformation, + StoredOAuthTokens, +} from '@modelcontextprotocol/client'; +import type { McpOAuthConfig } from '@maka/core/mcp'; + +/** Everything the provider persists for one server, as one JSON document. */ +export interface McpOAuthRecord { + /** Monotonic write version, stamped by the credential coordinator on + * every transition and validated against the read basis — the CAS handle + * that keeps an external writer from being silently overwritten. */ + version?: number; + /** Persisted revocation generation, owned by the credential coordinator. + * Logout/removal advances it and leaves the record as a tombstone that + * carries nothing else; every flow captures the generation on its first + * read and every later write verifies it — so a flow raced by a logout in + * ANOTHER process cannot resurrect revoked credentials. Deletion alone + * cannot encode cross-process revocation. */ + generation?: number; + /** The MCP server URL these credentials were issued for. Every read path + * refuses (and drops) the record when the configured URL no longer + * matches, so an offline edit of mcp.json cannot replay a token against + * a different endpoint. Absent only on records written before this field + * existed; they bind on their next save. */ + serverUrl?: string; + tokens?: StoredOAuthTokens; + clientInformation?: StoredOAuthClientInformation; + /** RFC 9728 / AS metadata discovered on a previous round — including the + * custom resource_metadata URL a 401's WWW-Authenticate advertised, which + * a later interactive login could not re-learn on its own. */ + discovery?: OAuthDiscoveryState; + codeVerifier?: string; + /** The exact redirect URL the pending authorization was started with. */ + pendingRedirectUrl?: string; + /** The server URL the pending authorization was started against, so the + * token exchange refuses to complete against a URL that changed under it. */ + pendingServerUrl?: string; + /** The OAuth state of the pending round, so a restarted app can rebind + * the loopback listener and still verify the browser's callback. */ + pendingState?: string; +} + +export interface McpOAuthStorage { + get(serverId: string): Promise; + set(serverId: string, record: McpOAuthRecord): Promise; + delete(serverId: string): Promise; + /** Optional atomic read-modify-write. The credential coordinator provides + * it on the storage views it hands to auth flows; base backends may omit + * it (callers fall back to read + set). */ + update?( + serverId: string, + apply: (record: McpOAuthRecord) => McpOAuthRecord, + ): Promise; + /** Optional compare-and-set keyed on the record's version basis + * (`null` asserts absence). Backends over a store with native CAS map it + * through so cross-process writers cannot be clobbered. */ + compareAndSet?( + serverId: string, + expectedVersion: number | null, + record: McpOAuthRecord, + ): Promise<'committed' | 'conflict' | 'gone'>; +} + +export function createMemoryMcpOAuthStorage(): McpOAuthStorage { + const records = new Map(); + return { + async get(serverId) { + const record = records.get(serverId); + return record ? structuredClone(record) : undefined; + }, + async set(serverId, record) { + records.set(serverId, structuredClone(record)); + }, + async delete(serverId) { + records.delete(serverId); + }, + }; +} + +/** Thrown (via the SDK's UnauthorizedError path) when a background connect + * would need the user in a browser. The manager maps it to `needs-auth`. */ +export class McpAuthRequiredError extends Error { + constructor(readonly serverId: string) { + super(`MCP server "${serverId}" requires interactive authorization`); + this.name = 'McpAuthRequiredError'; + } +} + +export interface McpOAuthProviderOptions { + serverId: string; + /** The configured MCP server URL. Credentials are bound to it: a stored + * record whose serverUrl differs is dropped instead of replayed. */ + serverUrl: string; + storage: McpOAuthStorage; + config?: McpOAuthConfig; + clientName: string; + clientVersion: string; + /** + * Absent for background connects: the provider then refuses interactive + * redirects with McpAuthRequiredError instead of opening anything. + * Present during a user-initiated login: the redirect URL the loopback + * callback server is listening on, plus a sink that receives the + * authorization URL for the caller to open in the system browser. + */ + interactive?: { + redirectUrl: string; + onAuthorizationUrl(url: URL): void; + /** OAuth `state` for the authorization URL; the callback listener + * verifies the round-trip before accepting a code. */ + state?: string; + }; +} + +/** Stands in for `redirectUrl` on background connects. The SDK reads an + * undefined redirectUrl as "non-interactive grant" and goes straight to the + * token endpoint — before ever consulting the stored refresh token — which + * turns every background connect with a known client into + * "Either provider.prepareTokenRequest() or authorizationCode is required". + * A defined value keeps the SDK on the authorization-code path: refresh + * runs first, and the interactive round it may then want is refused by + * redirectToAuthorization. The URL itself is never opened or sent in a + * token request. */ +const BACKGROUND_REDIRECT_URL = 'http://127.0.0.1/maka-mcp-oauth-noninteractive'; + +export class McpOAuthProvider implements OAuthClientProvider { + /** Present only when an interactive state was supplied — the SDK treats + * a defined method as "client uses state". */ + state?: () => string; + + constructor(private readonly options: McpOAuthProviderOptions) { + const state = options.interactive?.state; + if (state) this.state = () => state; + } + + get redirectUrl(): string { + return this.options.interactive?.redirectUrl ?? BACKGROUND_REDIRECT_URL; + } + + get clientMetadata(): OAuthClientMetadata { + return { + client_name: this.options.clientName, + client_uri: 'https://github.com/maka-agent/maka-agent', + software_id: 'maka-desktop', + software_version: this.options.clientVersion, + redirect_uris: this.options.interactive ? [this.options.interactive.redirectUrl] : [], + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + token_endpoint_auth_method: this.options.config?.clientSecret ? 'client_secret_post' : 'none', + ...(this.options.config?.scopes?.length + ? { scope: this.options.config.scopes.join(' ') } + : {}), + }; + } + + async clientInformation(): Promise { + const configured = this.options.config; + if (configured?.clientId) { + return { + client_id: configured.clientId, + ...(configured.clientSecret ? { client_secret: configured.clientSecret } : {}), + }; + } + const stored = (await this.read()).clientInformation; + // A background connect with no client at all is already decided: the + // interactive round is unavoidable. Refusing here (rather than at the + // redirect) stops the SDK from dynamically registering a throwaway + // client with no usable redirect URI. + if (!stored && !this.options.interactive) { + throw new McpAuthRequiredError(this.options.serverId); + } + return stored; + } + + async saveClientInformation(clientInformation: StoredOAuthClientInformation): Promise { + await this.mutate((record) => { + record.clientInformation = clientInformation; + }); + } + + async tokens(): Promise { + return (await this.read()).tokens; + } + + async saveTokens(tokens: StoredOAuthTokens): Promise { + await this.mutate((record) => { + record.tokens = tokens; + // A fresh token set settles any pending interactive round. + delete record.codeVerifier; + delete record.pendingRedirectUrl; + delete record.pendingServerUrl; + delete record.pendingState; + }); + } + + async redirectToAuthorization(authorizationUrl: URL): Promise { + const interactive = this.options.interactive; + if (!interactive) throw new McpAuthRequiredError(this.options.serverId); + interactive.onAuthorizationUrl(authorizationUrl); + } + + async saveCodeVerifier(codeVerifier: string): Promise { + // A background connect only reaches here on its way to refusing the + // redirect; persisting its verifier would clobber a live interactive + // round's pending record. + if (!this.options.interactive) return; + await this.mutate((record) => { + record.codeVerifier = codeVerifier; + record.pendingRedirectUrl = this.options.interactive?.redirectUrl; + record.pendingServerUrl = this.options.serverUrl; + record.pendingState = this.options.interactive?.state; + }); + } + + /** Persisted so an interactive login after a background 401 reuses the + * discovery that round already did — including a custom resource_metadata + * URL from WWW-Authenticate that a from-scratch discovery cannot find. */ + async discoveryState(): Promise { + return (await this.read()).discovery; + } + + async saveDiscoveryState(state: OAuthDiscoveryState): Promise { + await this.mutate((record) => { + // A dynamically registered client belongs to the authorization server + // that issued it. When discovery moves the resource to a DIFFERENT + // authorization server, carrying the old registration over would send + // one AS's client credentials (and any secret) to another. Static + // config-supplied clients are unaffected — they never live in the + // record. + const previousIssuer = record.discovery?.authorizationServerUrl; + const nextIssuer = state.authorizationServerUrl; + if (previousIssuer && nextIssuer && `${previousIssuer}` !== `${nextIssuer}`) { + delete record.clientInformation; + delete record.tokens; + } + record.discovery = state; + }); + } + + async codeVerifier(): Promise { + const value = (await this.read()).codeVerifier; + if (!value) { + throw new Error(`No pending authorization for MCP server "${this.options.serverId}"`); + } + return value; + } + + async invalidateCredentials( + scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery', + ): Promise { + if (scope === 'all') { + await this.options.storage.delete(this.options.serverId); + return; + } + await this.mutate((record) => { + if (scope === 'tokens') delete record.tokens; + if (scope === 'client') delete record.clientInformation; + if (scope === 'discovery') delete record.discovery; + if (scope === 'verifier') { + delete record.codeVerifier; + delete record.pendingRedirectUrl; + delete record.pendingServerUrl; + delete record.pendingState; + } + }); + } + + /** The redirect URL a pending authorization was started with, so the + * token exchange after the callback reuses the exact registered value. */ + async pendingRedirectUrl(): Promise { + return (await this.read()).pendingRedirectUrl; + } + + /** Reads the record, enforcing the endpoint binding: a record issued for + * a different server URL is dropped, never replayed — the config may have + * been edited (even offline) to point the same id at a new endpoint. A + * record carrying credential material with NO binding at all fails closed + * the same way: provenance cannot be established after first use, so a + * legacy or hand-written record is revoked rather than adopted — the user + * logs in again and the fresh record binds on its first save. */ + private async read(): Promise { + const record = await this.options.storage.get(this.options.serverId); + if (!record) return {}; + if (this.bindingMismatch(record)) { + await this.options.storage.delete(this.options.serverId); + return {}; + } + return record; + } + + private bindingMismatch(record: McpOAuthRecord): boolean { + const boundable = + record.tokens || + record.clientInformation || + record.codeVerifier || + record.discovery || + record.pendingRedirectUrl || + record.pendingServerUrl || + record.pendingState; + return Boolean(boundable) && record.serverUrl !== this.options.serverUrl; + } + + /** The same fail-closed binding rule as read(), applied to a mutation + * basis: a record bound elsewhere contributes NOTHING to the new write — + * only the coordinator's bookkeeping survives. Without this, an atomic + * update after an offline endpoint change would carry the old endpoint's + * tokens, client and discovery into the record it re-stamps for the new + * URL — a rebinding read() alone cannot prevent. */ + private stripUnbound(record: McpOAuthRecord): McpOAuthRecord { + if (!this.bindingMismatch(record)) return record; + const kept: McpOAuthRecord = {}; + if (record.version !== undefined) kept.version = record.version; + if (record.generation !== undefined) kept.generation = record.generation; + return kept; + } + + private async mutate(apply: (record: McpOAuthRecord) => void): Promise { + const stamp = (record: McpOAuthRecord): McpOAuthRecord => { + apply(record); + record.serverUrl = this.options.serverUrl; + return record; + }; + // The coordinator-provided storage view makes read-apply-write one + // atomic lane operation; plain backends (tests) fall back to two calls. + if (this.options.storage.update) { + await this.options.storage.update(this.options.serverId, (record) => + stamp(this.stripUnbound({ ...record })), + ); + return; + } + const record = await this.read(); + await this.options.storage.set(this.options.serverId, stamp(record)); + } +} diff --git a/packages/mcp/src/transport-security.ts b/packages/mcp/src/transport-security.ts new file mode 100644 index 0000000000..a81dc3060f --- /dev/null +++ b/packages/mcp/src/transport-security.ts @@ -0,0 +1,71 @@ +// packages/mcp/src/transport-security.ts +// +// Transport security is not network authority: cleartext-loopback is a +// TRUST decision, not a scheme rule, and only the user's own configuration +// confers it. A remotely supplied destination — a redirect Location, an +// OAuth metadata URL, an authorization endpoint — must not inherit the +// loopback exception unless the user pointed the server itself at loopback +// (a local trust root). +// +// The same provenance rule extends to `https` aimed BACK INTO the user's +// machine or network: a remote server's metadata can name +// `https://169.254.169.254/…` or `https://192.168.1.1/…` and the client +// would dutifully issue the request — blind SSRF from inside the network. +// Public https destinations pass (that is what OAuth endpoints look like); +// loopback and private-range IP LITERALS require the configured endpoint to +// be a matching local/internal trust root. Names that merely RESOLVE to +// private addresses are accepted risk: checking them would require a +// resolve here and could still re-resolve differently at request time. + +import { isLoopbackHost, isNonLoopbackCleartextHttp, isPrivateRangeHost } from '@maka/core/mcp'; + +export interface UrlProvenance { + configuredOrigin: string; + configuredOriginIsLoopback: boolean; + configuredOriginIsPrivate: boolean; +} + +export function urlProvenance(configuredServerUrl: URL): UrlProvenance { + return { + configuredOrigin: configuredServerUrl.origin, + configuredOriginIsLoopback: isLoopbackHost(configuredServerUrl.hostname), + configuredOriginIsPrivate: isPrivateRangeHost(configuredServerUrl.hostname), + }; +} + +export function assertTransportSecurity(url: URL, provenance: UrlProvenance): void { + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + throw new Error(`refused non-HTTP request to ${url.protocol}//`); + } + if (url.protocol === 'https:') { + if (url.origin === provenance.configuredOrigin) return; + if (isLoopbackHost(url.hostname)) { + if (provenance.configuredOriginIsLoopback) return; + throw new Error( + `refused remotely supplied loopback https destination ${url.origin}: ` + + 'only a user-configured loopback endpoint may target the local machine', + ); + } + if (isPrivateRangeHost(url.hostname)) { + if (provenance.configuredOriginIsLoopback || provenance.configuredOriginIsPrivate) return; + throw new Error( + `refused remotely supplied private-range https destination ${url.origin}: ` + + 'only a local or internal configured endpoint may target the private network', + ); + } + return; + } + if (isNonLoopbackCleartextHttp(url)) { + throw new Error( + `refused cleartext http request to ${url.origin}: non-loopback hosts require https`, + ); + } + // http + loopback from here on: allowed for the configured endpoint + // itself, or when the configured endpoint is a loopback trust root. + if (url.origin === provenance.configuredOrigin) return; + if (provenance.configuredOriginIsLoopback) return; + throw new Error( + `refused remotely supplied loopback http destination ${url.origin}: ` + + 'only a user-configured loopback endpoint may use cleartext', + ); +} diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index 731cd582d6..d64efb0f37 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -235,12 +235,10 @@ test('rejects corrupt files and unsafe or invalid configs', async () => { }), /http or https/u, ); - assert.throws( - () => - normalizeMcpConfig({ - version: 2, - mcpServers: { bad: { url: 'https://user:secret@example.com/mcp' } }, - }), + await assert.rejects( + createMcpConfigStore(await tempRoot()).upsert('bad', { + url: 'https://user:secret@example.com/mcp', + }), /embedded credentials/u, ); assert.throws(() => normalizeMcpConfig({ version: 3, mcpServers: {} }), /Unsupported/u); @@ -313,6 +311,63 @@ test('full replacement can migrate an existing version 1 wrapper to version 2', }); }); +test('refuses cleartext http for non-loopback hosts at the write boundary', async () => { + const store = createMcpConfigStore(await tempRoot()); + await assert.rejects( + store.upsert('bad', { url: 'http://example.com/mcp' }), + /https for non-loopback/u, + ); + for (const url of [ + 'http://127.0.0.1:8080/mcp', + 'http://localhost:3000/mcp', + 'https://example.com/mcp', + ]) { + await assert.doesNotReject(store.upsert('ok', { url })); + } +}); + +test('grandfathers a pre-existing cleartext server on read and keeps the file repairable', async () => { + // A single entry every prior release accepted must not brick the whole + // file: the page would come up empty and even the remove that could fix + // it would take the same throwing path. + const root = await tempRoot(); + const path = join(root, 'mcp.json'); + await writeFile( + path, + `${JSON.stringify({ + version: 2, + mcpServers: { + internal: { url: 'http://mcp.internal.corp/mcp', transport: 'auto' }, + good: { command: 'npx' }, + }, + })}\n`, + 'utf8', + ); + const store = createMcpConfigStore(root); + + const loaded = await store.get(); + assert.ok(loaded.mcpServers.internal); + assert.ok(loaded.mcpServers.good); + + // Repair paths stay open: removing either server works, and toggling the + // grandfathered entry (same URL) works. + await assert.doesNotReject(store.remove('good')); + await assert.doesNotReject( + store.upsert('internal', { url: 'http://mcp.internal.corp/mcp', enabled: false }), + ); + // Introducing or repointing a cleartext endpoint still refuses. + await assert.rejects( + store.upsert('internal', { url: 'http://other.internal.corp/mcp' }), + /https for non-loopback/u, + ); + await assert.rejects( + store.upsert('fresh', { url: 'http://example.com/mcp' }), + /https for non-loopback/u, + ); + await assert.doesNotReject(store.remove('internal')); + assert.deepEqual((await store.get()).mcpServers, {}); +}); + test('normalizes and bounds the remote oauth block', async () => { const normalized = normalizeMcpConfig({ version: 1, @@ -347,6 +402,28 @@ test('normalizes and bounds the remote oauth block', async () => { }), /clientId/u, ); + // Scopes join space-delimited on the wire and must be RFC 6749 §3.3 + // scope-tokens: an empty, whitespace-containing, control-carrying, + // quoted/backslashed or non-ASCII entry would silently change the + // requested grant or come back as invalid_scope far from the mistake. + for (const scopes of [ + [''], + ['read write'], + ['read', 'a\tb'], + ['read"admin'], + ['read\\admin'], + ['read\u0001admin'], + ['caf\u00e9'], + ]) { + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { bad: { url: 'https://example.com/mcp', oauth: { clientId: 'x', scopes } } }, + }), + /scope token/u, + ); + } // A clientSecret alone cannot form static client credentials. assert.throws( () => @@ -356,6 +433,28 @@ test('normalizes and bounds the remote oauth block', async () => { }), /clientId is required/u, ); + // Scopes join space-delimited on the wire and must be RFC 6749 §3.3 + // scope-tokens: an empty, whitespace-containing, control-carrying, + // quoted/backslashed or non-ASCII entry would silently change the + // requested grant or come back as invalid_scope far from the mistake. + for (const scopes of [ + [''], + ['read write'], + ['read', 'a\tb'], + ['read"admin'], + ['read\\admin'], + ['readadmin'], + ['café'], + ]) { + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { bad: { url: 'https://example.com/mcp', oauth: { clientId: 'x', scopes } } }, + }), + /scope token/u, + ); + } // stdio servers have no oauth block; unknown fields there stay rejected // by the stdio branch simply dropping them. const stdio = normalizeMcpConfig({ diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index de9ce6222e..2a205a935f 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -4,6 +4,7 @@ import { dirname, join } from 'node:path'; import { MCP_CONFIG_VERSION, createDefaultMcpConfig, + isNonLoopbackCleartextHttp, type McpConfigFile, type McpOAuthConfig, type McpProtocolPreference, @@ -30,6 +31,16 @@ export interface McpConfigStore { remove(serverId: string): Promise; } +/** Thrown by insert when the id is taken. Same-process callers (the IPC + * layer) match on instanceof and answer the renderer with a typed + * envelope; the message never has to carry a machine-readable code. */ +export class McpServerExistsError extends Error { + constructor(readonly serverId: string) { + super(`MCP server "${serverId}" already exists`); + this.name = 'McpServerExistsError'; + } +} + export function createMcpConfigStore(workspaceRoot: string): McpConfigStore { return new FileMcpConfigStore(join(workspaceRoot, 'mcp.json')); } @@ -66,6 +77,7 @@ class FileMcpConfigStore implements McpConfigStore { const normalized = normalizeMcpConfig(config); return this.serial(async () => { await this.assertCurrentVersionCanBeReplaced(); + enforceEndpointPolicyOnChanges(await this.tryRead(), normalized); await this.write(normalized); return normalized; }); @@ -75,6 +87,7 @@ class FileMcpConfigStore implements McpConfigStore { return this.serial(async () => { const current = await this.readOrCreate(); const next = normalizeMcpConfig(apply(current)); + enforceEndpointPolicyOnChanges(current, next); await this.write(next); return next; }); @@ -88,6 +101,7 @@ class FileMcpConfigStore implements McpConfigStore { version: MCP_CONFIG_VERSION, mcpServers: { ...current.mcpServers, [serverId]: config }, }); + enforceEndpointPolicyOnChanges(current, next); await this.write(next); return next; }); @@ -104,6 +118,16 @@ class FileMcpConfigStore implements McpConfigStore { }); } + private async tryRead(): Promise { + try { + return await this.readOrCreate(); + } catch { + // A malformed file is recovered by full replacement; policy then + // applies to every server in the replacement. + return undefined; + } + } + private async readOrCreate(): Promise { try { const text = await readFile(this.path, 'utf8'); @@ -173,6 +197,39 @@ class FileMcpConfigStore implements McpConfigStore { } } +/** Endpoint security policy, enforced at the WRITE boundary for new or + * repointed endpoints only. Reads grandfather whatever earlier releases + * accepted: a single legacy `http://` entry must not make the whole file — + * and every other server in it — unreadable and unrepairable from the app. + * The transport layer still refuses to CONNECT such an endpoint, so a + * grandfathered entry surfaces as a per-server error, not a working + * cleartext channel. */ +export function assertMcpEndpointPolicy(server: McpServerConfig, serverId: string): void { + if (!('url' in server)) return; + const parsed = new URL(server.url); + if (isNonLoopbackCleartextHttp(parsed)) { + // A remote MCP endpoint carries bearer tokens and tool payloads. + throw new Error(`${serverId}.url must use https for non-loopback hosts`); + } + if (parsed.username || parsed.password) { + throw new Error(`${serverId}.url must not contain embedded credentials; use headers instead`); + } +} + +function enforceEndpointPolicyOnChanges( + previous: McpConfigFile | undefined, + next: McpConfigFile, +): void { + for (const [serverId, server] of Object.entries(next.mcpServers)) { + if (!('url' in server)) continue; + const before = previous?.mcpServers[serverId]; + const beforeUrl = before && 'url' in before ? before.url : undefined; + // Enabling/disabling or editing headers on a grandfathered entry stays + // possible; introducing or repointing an endpoint takes the policy. + if (server.url !== beforeUrl) assertMcpEndpointPolicy(server, serverId); + } +} + function normalizeServer( value: unknown, serverId: string, @@ -209,9 +266,6 @@ function normalizeServer( if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { throw new Error(`${serverId}.url must use http or https`); } - if (parsed.username || parsed.password) { - throw new Error(`${serverId}.url must not contain embedded credentials; use headers instead`); - } const transport = value.transport ?? 'auto'; if (transport !== 'auto' && transport !== 'streamable-http' && transport !== 'sse') { throw new Error(`${serverId}.transport is invalid`); @@ -255,7 +309,16 @@ function normalizeOAuth(value: unknown, serverId: string): McpOAuthConfig { throw new Error(`${serverId}.oauth.clientId is required when clientSecret is configured`); } if (value.scopes !== undefined) { - result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`); + result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`).map((scope, index) => { + // RFC 6749 §3.3 scope-token: printable ASCII except space, quote and + // backslash. The list joins space-delimited on the wire, so an entry + // outside the grammar would silently change the requested grant or be + // rejected as invalid_scope far from the config mistake. + if (!/^[\x21\x23-\x5B\x5D-\x7E]+$/u.test(scope)) { + throw new Error(`${serverId}.oauth.scopes[${index}] must be a non-empty scope token`); + } + return scope; + }); } if (value.callbackPort !== undefined) { if (