From 60e609d712c019e78b033a0b780a72a2b4878e47 Mon Sep 17 00:00:00 2001 From: GabrielDrapor Date: Wed, 12 Aug 2026 20:18:16 +0800 Subject: [PATCH 1/2] feat(mcp): OAuth for remote MCP servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remote servers that answer 401 now surface as 需要登录 instead of a connection error. Background connects run the SDK's OAuth client against stored tokens: silent refresh works (the provider always defines a redirectUrl — leaving it undefined routes the SDK into the non-interactive token path before it ever reads the refresh token), and a connect that would need the user refuses before dynamic registration and maps to the new needs-auth state. Interactive rounds live in startAuthorization / finishAuthorization: discovery reuses the state the background 401 round persisted (including a custom resource_metadata URL from WWW-Authenticate), a challenge probe — GET, then an initialize POST at the SDK's current protocol version, pressing on past parameterless challenges — carries the 401's scope into the authorization request, dynamic client registration (static clientId/clientSecret config as fallback), PKCE, and a persisted verifier + state so the exchange survives restarts. Credentials follow the endpoint they were issued for, and deletion is terminal. The stored record carries the server URL it was minted against and every read path drops a mismatched record (an offline mcp.json edit cannot replay a token against a new endpoint). All credential state transitions flow through one coordinator: a per-server operation lane carries every read, write and delete; removing a server, changing its URL, or logging out bumps a credential epoch in-flight flows are pinned to — so a stale flow can neither read old material past a queued delete, write a refresh result over a cleared record, nor delete what a newer flow just stored. Every write stamps a monotonically increasing version validated against the basis it read, and where the backing store exposes compare-and-set, an external edit trips the check instead of being silently overwritten. A failed delete holds the server in error rather than connecting anyway, and a 401 after connect (recognized through the scrub boundary, which preserves the transport's status code) marks the server needs-auth and bumps the tool-snapshot revision so stale capabilities drop out of the Runtime Host. Authorization stays bound to where it came from. The authorization URL a server supplies is checked against the provenance of the configured endpoint — transport security is not network authority, so remotely-supplied loopback cleartext http is refused unless the user themselves configured a loopback origin. The OAuth callback travels as a typed payload that preserves the `iss` parameter for the SDK's RFC 9207 issuer mix-up check. And a configured Authorization header and OAuth are mutually exclusive on the wire: once OAuth owns a connection's authorization — configured, or evidenced by stored credentials — the static header is dropped rather than raced against the bearer token. Secrets stay contained on every path out. Where they live is enumerated once, in @maka/core/mcp-secrets — the same location plan the desktop IPC guard masks by — so the scrubber and the boundary cannot drift. Requests: one scoped fetch carries every remote and OAuth request; configured resource headers ride only the endpoint's own origin, any redirect hop that crosses an origin sheds Authorization/Cookie for the rest of the chain, and no hop may downgrade to non-loopback cleartext http. Messages and payloads: errors, status strings, stderr tails, tool-call results, structured content and tool descriptors — object keys included, since a server can smuggle a credential through a property name — are all scrubbed of the config's credential values and of everything harvested from OAuth storage traffic (access, refresh and id tokens, registered client secrets, the PKCE verifier, and the in-flight authorization code during its exchange). A value long enough to be unambiguous is substituted in place; a message containing a credential too short to splice out is withheld wholesale, because the boundary allows no third option. Tested end to end against a real authorization-server fixture, including silent refresh, revoked-session recovery, replay refusal, logout-during-refresh finality in both interleavings, external-writer CAS refusal, forged- and genuine-issuer callbacks, Authorization exclusion under stored credentials, reflected-secret scrubbing across token endpoint / resource error / tool error / success payload / metadata / object-key / authorization-code paths, short-secret withholding, bearer stripping across cross-origin redirects, cleartext-downgrade refusal, and challenge scope propagation for GET, bare-GET and strict POST-only servers. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj --- packages/core/src/mcp.ts | 58 +- packages/mcp/src/__fixtures__/stdio-server.ts | 5 +- .../__tests__/credential-coordinator.test.ts | 149 ++ .../src/__tests__/manager-fallback.test.ts | 20 +- packages/mcp/src/__tests__/manager.test.ts | 221 +++ packages/mcp/src/__tests__/oauth.test.ts | 1665 +++++++++++++++++ .../src/__tests__/transport-security.test.ts | 94 + packages/mcp/src/credential-coordinator.ts | 239 +++ packages/mcp/src/index.ts | 1172 +++++++++++- packages/mcp/src/oauth.ts | 352 ++++ packages/mcp/src/transport-security.ts | 71 + .../src/__tests__/mcp-config-store.test.ts | 89 +- packages/storage/src/mcp-config-store.ts | 61 +- 13 files changed, 4144 insertions(+), 52 deletions(-) create mode 100644 packages/mcp/src/__tests__/credential-coordinator.test.ts create mode 100644 packages/mcp/src/__tests__/oauth.test.ts create mode 100644 packages/mcp/src/__tests__/transport-security.test.ts create mode 100644 packages/mcp/src/credential-coordinator.ts create mode 100644 packages/mcp/src/oauth.ts create mode 100644 packages/mcp/src/transport-security.ts diff --git a/packages/core/src/mcp.ts b/packages/core/src/mcp.ts index c15558966c..e4990cf1c3 100644 --- a/packages/core/src/mcp.ts +++ b/packages/core/src/mcp.ts @@ -42,7 +42,60 @@ 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); +} + +export type McpConnectionState = + | 'disabled' + | 'disconnected' + | 'connecting' + | 'connected' + | 'needs-auth' + | 'error'; export interface McpNegotiatedProtocol { era: 'legacy' | 'modern'; @@ -95,6 +148,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/index.ts b/packages/mcp/src/index.ts index f0ee5f9f35..3a08c99b28 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,44 @@ 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 { + 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 +110,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 +122,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 +189,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 +233,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 +248,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 +257,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 +377,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 +462,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 +509,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 +764,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 +784,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 +826,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 +852,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 +953,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 +1000,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 +1013,7 @@ export class McpClientManager { tools: initialRefresh.descriptors, error: projectConnectionDiagnostics(entry), stderrTail: entry.status.stderrTail, + ...(authenticated !== undefined ? { authenticated } : {}), updatedAt: this.now(), }); if ( @@ -769,7 +1074,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 +1103,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 +1120,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 +1129,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 +1189,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 +1210,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 +1684,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 +1807,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 +1820,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 +1844,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 +1854,7 @@ export class McpClientManager { this.bindingManagerId, state.connectionGeneration, entry.toolSnapshot, + (descriptor) => deepScrub(descriptor, inventory), ); latestSnapshot = snapshot; if (notificationStateMatches) notificationState.lastPassCompletedAt = this.now(); @@ -1185,9 +1948,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 +2140,7 @@ function shouldFallbackToLegacySse(options: { ); } -function createStreamableHandshakeEvidence(): { +function createStreamableHandshakeEvidence(base: FetchLike = globalThis.fetch): { fetch: FetchLike; hasAcceptedInitialize(): boolean; } { @@ -1361,7 +2154,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 +2203,8 @@ function createToolSnapshot( managerId: string, connectionGeneration: number, previousEntries?: ReadonlyMap, + scrubDescriptor: (descriptor: McpToolDescriptor) => McpToolDescriptor = (descriptor) => + descriptor, ): { entries: Map; descriptors: McpToolDescriptor[]; @@ -1435,7 +2230,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 +2372,7 @@ export function buildStdioEnvironment( function attachStderrTail( transport: StdioClientTransport, entry: Connection, + secrets: SecretInventory, onUpdate: () => void, ): void { let pending = ''; @@ -1585,7 +2381,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 +2456,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 +2504,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 +2555,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 +2810,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..8802c2656e 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( () => diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index de9ce6222e..345fb3b013 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, @@ -66,6 +67,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 +77,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 +91,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 +108,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 +187,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 +256,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 +299,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 ( From 31cbc8f3829533ce8d6e8d85ee251f4099f8b995 Mon Sep 17 00:00:00 2001 From: GabrielDrapor Date: Wed, 12 Aug 2026 20:20:10 +0800 Subject: [PATCH 2/2] feat(desktop): MCP OAuth login flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit login() binds an ephemeral 127.0.0.1 callback listener (oauth.callbackPort pins it for statically registered clients) and opens the system browser per RFC 8252 — refusing any authorization URL that is not https or loopback http, since the URL comes from remote OAuth metadata and a cleartext login off the machine would hand the code exchange to the network. The listener verifies the OAuth state round-trip before reading either the code or an error parameter, so a forged access_denied on loopback cannot abort a real login — and it settles with a typed payload that carries the code together with the `iss` parameter, feeding the SDK's RFC 9207 issuer mix-up check instead of dropping it on the floor. When a round does fail, only an allowlisted RFC 6749 error code crosses toward the renderer; the server-controlled error_description stays in the browser tab. A login round interrupted by an app restart resumes at boot, from persisted state alone: the resume claims its in-progress guard before any await, rebinds the listener before and independent of the connect/publish chain, treats a since-occupied port as nothing-to-resume rather than a failure, and awaits readiness only for the final token exchange. One deadline covers the complete round — discovery, the browser wait, and the token exchange — so a remote endpoint that accepts a connection and never answers cannot hold the in-progress guard or the loopback listener; the listener closes on every exit. The deadline's abort signal travels INTO the round — the manager aborts its requests and fences its late storage writes on it — so a first round completing after its timeout can neither exchange its code nor overwrite the state of a newer round. Tokens live in the shared CredentialStore (credentials.json, 0600) — never in mcp.json — and the store exposes compare-and-set over the stored record, so the runtime's credential coordinator refuses to clobber a record something else edited underneath it. The renderer reaches all of this through the same scoped Runtime Host seam as every other MCP method: the handlers live on ScopedIpcMain, whose first argument is the host ref, and a source-level contract test pins every mcp: channel to invokeActiveRuntimeHost so a raw invoke cannot sneak back in. Removal is transactional in the safe direction: mcp:remove and cancelled installs drop the server's stored credentials FIRST and abort — config intact, retryable — if that fails, so a same-id re-add can never inherit an orphaned token. The store gains an atomic insert for the dialog's add path — a taken id answers as a typed { status: 'exists' } envelope (own-property checked) rather than as prose fished out of a flattened IPC error string. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj --- apps/desktop/package.json | 1 + .../src/main/__tests__/mcp-ipc-main.test.ts | 690 +++++++++++++--- .../__tests__/mcp-oauth-controller.test.ts | 747 ++++++++++++++++++ .../main/__tests__/mcp-preload-scope.test.ts | 30 + apps/desktop/src/main/mcp-ipc-main.ts | 286 ++++++- apps/desktop/src/main/mcp-oauth-controller.ts | 502 ++++++++++++ apps/desktop/src/main/runtime-host-boot.ts | 53 +- apps/desktop/src/preload/bridge-contract.d.ts | 8 + apps/desktop/src/preload/preload.ts | 17 + package-lock.json | 1 + ...untime-host-capability-provider-command.ts | 11 +- packages/core/src/mcp.ts | 5 + packages/mcp/src/credential-oauth-storage.ts | 91 +++ packages/mcp/src/index.ts | 4 + .../src/__tests__/mcp-config-store.test.ts | 22 + packages/storage/src/mcp-config-store.ts | 10 + 16 files changed, 2339 insertions(+), 139 deletions(-) create mode 100644 apps/desktop/src/main/__tests__/mcp-oauth-controller.test.ts create mode 100644 apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts create mode 100644 apps/desktop/src/main/mcp-oauth-controller.ts create mode 100644 packages/mcp/src/credential-oauth-storage.ts 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 e4990cf1c3..50a905998e 100644 --- a/packages/core/src/mcp.ts +++ b/packages/core/src/mcp.ts @@ -89,6 +89,11 @@ 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' 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 3a08c99b28..a1e1287243 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -71,6 +71,10 @@ import { } from './oauth.js'; export { McpToolCallError } from './tool-call-error.js'; +export { + createCredentialMcpOAuthStorage, + type McpCredentialSecretStore, +} from './credential-oauth-storage.js'; export { createMemoryMcpOAuthStorage, McpAuthRequiredError, diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index 8802c2656e..d64efb0f37 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -433,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 345fb3b013..2a205a935f 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -31,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')); }