diff --git a/.maka-shots/2920-callback-failure.png b/.maka-shots/2920-callback-failure.png new file mode 100644 index 0000000000..35c5b51ab2 Binary files /dev/null and b/.maka-shots/2920-callback-failure.png differ diff --git a/.maka-shots/2920-callback-success.png b/.maka-shots/2920-callback-success.png new file mode 100644 index 0000000000..7d0ed9b71f Binary files /dev/null and b/.maka-shots/2920-callback-success.png differ diff --git a/.maka-shots/2920-login-denied.png b/.maka-shots/2920-login-denied.png new file mode 100644 index 0000000000..00e8dd5ae9 Binary files /dev/null and b/.maka-shots/2920-login-denied.png differ diff --git a/.maka-shots/2921-editor-live-validation.png b/.maka-shots/2921-editor-live-validation.png new file mode 100644 index 0000000000..b83f222900 Binary files /dev/null and b/.maka-shots/2921-editor-live-validation.png differ diff --git a/.maka-shots/2921-inspector-authenticated.png b/.maka-shots/2921-inspector-authenticated.png new file mode 100644 index 0000000000..ff2383a13b Binary files /dev/null and b/.maka-shots/2921-inspector-authenticated.png differ diff --git a/.maka-shots/2921-inspector-needs-auth.png b/.maka-shots/2921-inspector-needs-auth.png new file mode 100644 index 0000000000..00c62393b1 Binary files /dev/null and b/.maka-shots/2921-inspector-needs-auth.png differ diff --git a/.maka-shots/after-dialog.png b/.maka-shots/after-dialog.png new file mode 100644 index 0000000000..9977cad007 Binary files /dev/null and b/.maka-shots/after-dialog.png differ diff --git a/.maka-shots/before-dialog.png b/.maka-shots/before-dialog.png new file mode 100644 index 0000000000..8b563a74bb Binary files /dev/null and b/.maka-shots/before-dialog.png differ 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 fc071e5c02..1f928ebf68 100644 --- a/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts @@ -15,6 +15,11 @@ test('MCP IPC commits config before publishing capabilities and emitting status' ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, store: { get: async () => config, + transform: async (apply) => { + calls.push('store'); + config = apply(config); + return config; + }, set: async (next) => { calls.push('store'); config = next; return next; }, upsert: async (serverId, server) => { calls.push('store'); @@ -85,15 +90,19 @@ test('MCP market cancellation waits for an in-flight config write before rolling ipcMain: { handle(channel, handler) { handlers.set(channel, handler as (...args: any[]) => Promise); } }, store: { get: async () => config, - set: async (next) => { config = next; return next; }, - upsert: async (serverId, server) => { + transform: async (apply) => { calls.push('write:start'); markWriteStarted(); await writeGate; - config = { version: MCP_CONFIG_VERSION, mcpServers: { ...config.mcpServers, [serverId]: server } }; + 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; @@ -141,6 +150,10 @@ test('MCP config commit is not rolled back by a capability publication failure', }, store: { get: async () => config, + transform: async (apply) => { + config = apply(config); + return config; + }, set: async (next) => { config = next; return next; @@ -177,3 +190,121 @@ test('MCP config commit is not rolled back by a capability publication failure', 'Host disconnected', ]); }); + +test('MCP IPC redacts clientSecret toward the renderer and restores the sentinel from disk', 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' }, + }, + scratch: { + command: 'npx', + // An arbitrary flag name hiding a pattern-recognized token: the + // value marks it as a secret, not the name. + args: ['server', '--custom=sk-ant-api03-abcdef123456'], + env: { API_TOKEN: 'scratch-token' }, + }, + }, + }; + const synced: McpConfigFile[] = []; + 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 (serverId) => { + const { [serverId]: _removed, ...mcpServers } = config.mcpServers; + config = { version: MCP_CONFIG_VERSION, mcpServers }; + return config; + }, + }, + manager: { + cancelConnect: () => false, + sync: async (next) => { + synced.push(next); + }, + statuses: () => [], + test: async () => { + throw new Error('not used'); + }, + }, + ensureReady: async () => {}, + publishCapabilities: async () => {}, + onPublicationError: () => {}, + emitChanged: () => {}, + }); + + const getConfig = handlers.get('mcp:getConfig'); + assert.ok(getConfig); + const seen = await getConfig({}); + const notion = seen.mcpServers.notion; + assert.ok(notion && 'url' in notion); + assert.notEqual(notion.oauth?.clientSecret, 'real-secret'); + assert.ok(notion.oauth?.clientSecret); + const seenScratch = seen.mcpServers.scratch; + assert.ok(seenScratch && 'command' in seenScratch); + assert.ok(!seenScratch.args?.some((arg: string) => arg.includes('sk-ant-api03-abcdef123456'))); + + // The renderer round-trips the masked arg unchanged; the store gets the + // real token back from disk. + const upsertScratch = handlers.get('mcp:upsert'); + assert.ok(upsertScratch); + await upsertScratch({}, 'scratch', { ...seenScratch, enabled: false }); + const storedScratch = config.mcpServers.scratch; + assert.ok(storedScratch && 'command' in storedScratch); + assert.deepEqual(storedScratch.args, ['server', '--custom=sk-ant-api03-abcdef123456']); + + // The renderer edits the redacted config and sends the sentinel back: + // the store must get the real secret, the renderer only the sentinel. + const upsert = handlers.get('mcp:upsert'); + assert.ok(upsert); + const returned = await upsert({}, 'notion', { ...notion, transport: 'sse' }); + 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 echoed = returned.mcpServers.notion; + assert.ok(echoed && 'url' in echoed); + assert.notEqual(echoed.oauth?.clientSecret, 'real-secret'); + + // Removing or cancelling an unrelated server also returns a full config + // crossing toward the renderer — the survivors' secrets stay sentinels. + const remove = handlers.get('mcp:remove'); + assert.ok(remove); + const afterRemove = await remove({}, 'scratch'); + const survivorAfterRemove = afterRemove.mcpServers.notion; + assert.ok(survivorAfterRemove && 'url' in survivorAfterRemove); + assert.ok(survivorAfterRemove.oauth?.clientSecret); + assert.notEqual(survivorAfterRemove.oauth?.clientSecret, 'real-secret'); + + config = { + version: MCP_CONFIG_VERSION, + mcpServers: { ...config.mcpServers, doomed: { command: 'npx' } }, + }; + const cancelInstall = handlers.get('mcp:cancelInstall'); + assert.ok(cancelInstall); + const afterCancel = await cancelInstall({}, 'doomed'); + assert.equal(afterCancel.mcpServers.doomed, undefined); + const survivorAfterCancel = afterCancel.mcpServers.notion; + assert.ok(survivorAfterCancel && 'url' in survivorAfterCancel); + assert.ok(survivorAfterCancel.oauth?.clientSecret); + assert.notEqual(survivorAfterCancel.oauth?.clientSecret, 'real-secret'); +}); diff --git a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts index 3f1260f43d..a46e8afe45 100644 --- a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts @@ -108,6 +108,47 @@ test('the MCP catalog opts every bundled remote entry into auto negotiation', () } }); +test('an edit that does not touch OAuth preserves the block through save', () => { + const stored = { + enabled: true, + url: 'https://mcp.notion.com/mcp', + transport: 'auto' as const, + headers: { 'X-Workspace': 'w1' }, + oauth: { + clientId: 'abc', + clientSecret: ' maka-mcp-secret ', + scopes: ['read'], + callbackPort: 33389, + }, + }; + const draft = mcpDraftFromConfig('notion', stored); + const saved = mcpConfigFromDraft(withMcpDraftTransport(draft, 'sse'), copy); + assert.ok('url' in saved); + assert.equal(saved.transport, 'sse'); + assert.deepEqual(saved.oauth, stored.oauth); + assert.deepEqual(saved.headers, stored.headers); +}); + +test('editing does not invent an oauth block for servers that never had one', () => { + const saved = mcpConfigFromDraft( + mcpDraftFromConfig('plain', { url: 'https://example.com/mcp', enabled: true }), + copy, + ); + assert.ok('url' in saved && !('oauth' in saved)); +}); + +test('a stdio config round-trips through the command-line field', () => { + const stored = { + enabled: true, + command: 'npx', + args: ['-y', 'server', '--flag', 'a b'], + cwd: '/tmp/work', + env: { TOKEN: 'secret' }, + }; + const saved = mcpConfigFromDraft(mcpDraftFromConfig('local', stored), copy); + assert.deepEqual(saved, stored); +}); + test('status copy presents only a live connected negotiated protocol', () => { const status: McpServerStatus = { serverId: 'remote', diff --git a/apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts b/apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts new file mode 100644 index 0000000000..8488dcfc1b --- /dev/null +++ b/apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts @@ -0,0 +1,325 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { MCP_CONFIG_VERSION, type McpConfigFile } from '@maka/core/mcp'; +import { + mcpSecretMarker, + McpSecretRestoreError, + redactMcpConfigSecrets, + restoreMcpConfigSecrets, + restoreMcpServerSecret, +} from '../mcp-secret-guard.js'; + +const withSecret = (secret?: string): McpConfigFile => ({ + version: MCP_CONFIG_VERSION, + mcpServers: { + notion: { + url: 'https://mcp.notion.com/mcp', + transport: 'streamable-http', + ...(secret ? { oauth: { clientId: 'abc', clientSecret: secret } } : { oauth: { clientId: 'abc' } }), + }, + local: { command: 'npx', args: ['server'] }, + }, +}); + +describe('MCP secret redaction', () => { + it('masks all remote header values and restores them only for the same URL', () => { + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + api: { + url: 'https://api.example.com/mcp', + headers: { Authorization: 'Bearer live-token', 'X-Workspace': 'ws-1' }, + }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.api; + assert.ok(server && 'url' in server); + // Every header value is credential-position: masked, keys readable. + assert.deepEqual(server.headers, { + Authorization: mcpSecretMarker('header.Authorization'), + 'X-Workspace': mcpSecretMarker('header.X-Workspace'), + }); + + const restored = restoreMcpConfigSecrets(redacted, previous); + const restoredServer = restored.mcpServers.api; + assert.ok(restoredServer && 'url' in restoredServer); + assert.deepEqual(restoredServer.headers, { + Authorization: 'Bearer live-token', + 'X-Workspace': 'ws-1', + }); + + // A renderer that kept the sentinel but repointed the URL is rejected — + // silently dropping the headers would destroy stored values unnoticed. + const repointed = structuredClone(redacted); + const repointedServer = repointed.mcpServers.api; + assert.ok(repointedServer && 'url' in repointedServer); + repointedServer.url = 'https://attacker.example/mcp'; + assert.throws(() => restoreMcpConfigSecrets(repointed, previous), McpSecretRestoreError); + }); + + it('treats an unnormalized stored URL as the same endpoint', () => { + // mcp.json may carry a hand-written URL WHATWG would normalize (no path + // slash). The redacted round-trip normalizes; the identity comparison + // must not read that as an endpoint change and reject the restore. + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + api: { + url: 'https://api.example.com?api_key=qk-1', + headers: { Authorization: 'Bearer live-token' }, + oauth: { clientId: 'abc', clientSecret: 'real-secret' }, + }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const restored = restoreMcpConfigSecrets(redacted, previous); + const server = restored.mcpServers.api; + assert.ok(server && 'url' in server); + assert.deepEqual(server.headers, { Authorization: 'Bearer live-token' }); + assert.equal(server.oauth?.clientSecret, 'real-secret'); + assert.equal(new URL(server.url).searchParams.get('api_key'), 'qk-1'); + }); + + it('masks every stdio env value and binds restore to the whole launch basis', () => { + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + local: { + command: 'npx', + args: ['server'], + cwd: '/srv/app', + env: { PGPASSWORD: 'pg-secret', NODE_ENV: 'production' }, + }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.local; + assert.ok(server && 'command' in server); + // No key heuristic: PGPASSWORD would not match one, so every value is + // masked — the boundary is absolute. + assert.deepEqual(server.env, { + PGPASSWORD: mcpSecretMarker('env.PGPASSWORD'), + NODE_ENV: mcpSecretMarker('env.NODE_ENV'), + }); + + const restored = restoreMcpConfigSecrets(redacted, previous); + const restoredServer = restored.mcpServers.local; + assert.ok(restoredServer && 'command' in restoredServer); + assert.deepEqual(restoredServer.env, { PGPASSWORD: 'pg-secret', NODE_ENV: 'production' }); + + // Changing ANY part of the launch basis — command, cwd, or another env + // value — rejects the write: an ordinary edit must never silently + // delete every stored env value. + for (const mutate of [ + (s: Record) => { + s.command = 'curl'; + }, + (s: Record) => { + s.cwd = '/tmp'; + }, + (s: Record) => { + (s.env as Record).NODE_ENV = 'test'; + }, + ]) { + const repointed = structuredClone(redacted); + const target = repointed.mcpServers.local as unknown as Record; + mutate(target); + assert.throws(() => restoreMcpConfigSecrets(repointed, previous), McpSecretRestoreError); + } + }); + + it('masks credential-bearing args and binds their restore to the command line', () => { + // `--custom` is not a sensitive name — the VALUE is what marks it: a + // pattern-recognized token behind any flag prefix must mask too. + const priorArgs = [ + 'server', + '--token', + 'tok-abc123', + '--api-key=key-xyz789', + '--custom=sk-ant-api03-abcdef123456', + '--verbose', + ]; + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + local: { command: 'npx', args: priorArgs }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.local; + assert.ok(server && 'command' in server); + assert.deepEqual(server.args, [ + 'server', + '--token', + mcpSecretMarker('arg.2'), + `--api-key=${mcpSecretMarker('argflag.3')}`, + `--custom=${mcpSecretMarker('argflag.4')}`, + '--verbose', + ]); + + const restored = restoreMcpConfigSecrets(redacted, previous); + const restoredServer = restored.mcpServers.local; + assert.ok(restoredServer && 'command' in restoredServer); + assert.deepEqual(restoredServer.args, priorArgs); + + // A repointed command rejects: dropping the bare sentinel would leave + // its introducing flag consuming the next argument (`--token --verbose`). + const repointed = structuredClone(redacted); + const repointedServer = repointed.mcpServers.local; + assert.ok(repointedServer && 'command' in repointedServer); + repointedServer.command = 'curl'; + assert.throws(() => restoreMcpConfigSecrets(repointed, previous), McpSecretRestoreError); + }); + + it('masks sensitive URL query values and binds their restore to the rest of the URL', () => { + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + api: { + url: 'https://api.example.com/mcp?api_key=qk-secret-1®ion=eu', + }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.api; + assert.ok(server && 'url' in server); + const maskedUrl = new URL(server.url); + assert.equal(maskedUrl.searchParams.get('api_key'), mcpSecretMarker('query.api_key.0')); + assert.equal(maskedUrl.searchParams.get('region'), 'eu'); + + const restored = restoreMcpConfigSecrets(redacted, previous); + const restoredServer = restored.mcpServers.api; + assert.ok(restoredServer && 'url' in restoredServer); + assert.equal(new URL(restoredServer.url).searchParams.get('api_key'), 'qk-secret-1'); + + // Same sentinel, different path — the write is rejected. + const repointed = structuredClone(redacted); + const repointedServer = repointed.mcpServers.api; + assert.ok(repointedServer && 'url' in repointedServer); + repointedServer.url = repointedServer.url.replace('/mcp', '/exfil'); + assert.throws(() => restoreMcpConfigSecrets(repointed, previous), McpSecretRestoreError); + }); + + it('treats cosmetic query-encoding differences as the same endpoint', () => { + // The stored URL encodes a space as %20; URLSearchParams re-serializes + // it as +. The endpoint did not change — masked headers and the + // clientSecret must survive the round-trip. + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + api: { + url: 'https://api.example.com/mcp?label=a%20b&api_key=qk-secret-1', + headers: { Authorization: 'Bearer live-token' }, + oauth: { clientId: 'abc', clientSecret: 'real-secret' }, + }, + }, + }; + const restored = restoreMcpConfigSecrets(redactMcpConfigSecrets(previous), previous); + const server = restored.mcpServers.api; + assert.ok(server && 'url' in server); + assert.deepEqual(server.headers, { Authorization: 'Bearer live-token' }); + assert.equal(server.oauth?.clientSecret, 'real-secret'); + assert.equal(new URL(server.url).searchParams.get('api_key'), 'qk-secret-1'); + assert.equal(new URL(server.url).searchParams.get('label'), 'a b'); + }); + + it('masks and restores repeated sensitive query keys per occurrence', () => { + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + api: { url: 'https://api.example.com/mcp?token=a1a1®ion=eu&token=b2b2' }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.api; + assert.ok(server && 'url' in server); + const masked = new URL(server.url); + // Both occurrences survive, each bound to its own position. + assert.deepEqual(masked.searchParams.getAll('token'), [ + mcpSecretMarker('query.token.0'), + mcpSecretMarker('query.token.1'), + ]); + assert.equal(masked.searchParams.get('region'), 'eu'); + + const restored = restoreMcpConfigSecrets(redacted, previous); + const restoredServer = restored.mcpServers.api; + assert.ok(restoredServer && 'url' in restoredServer); + assert.deepEqual(new URL(restoredServer.url).searchParams.getAll('token'), ['a1a1', 'b2b2']); + }); + + it('rejects a marker moved to a different position', () => { + const previous: McpConfigFile = { + version: MCP_CONFIG_VERSION, + mcpServers: { + local: { command: 'npx', env: { PGPASSWORD: 'pg-secret', OTHER: 'plain' } }, + }, + }; + const redacted = redactMcpConfigSecrets(previous); + const server = redacted.mcpServers.local; + assert.ok(server && 'command' in server); + // A renderer swaps PGPASSWORD's marker into OTHER: the marker is bound + // to its position and must not act as a wildcard anywhere else. + const swapped = structuredClone(redacted); + const swappedServer = swapped.mcpServers.local; + assert.ok(swappedServer && 'command' in swappedServer && swappedServer.env); + swappedServer.env.OTHER = swappedServer.env.PGPASSWORD as string; + assert.throws(() => restoreMcpConfigSecrets(swapped, previous), McpSecretRestoreError); + }); + + it('replaces a stored clientSecret with the sentinel and leaves the rest', () => { + const redacted = redactMcpConfigSecrets(withSecret('real-secret')); + const notion = redacted.mcpServers.notion; + assert.ok(notion && 'url' in notion); + assert.equal(notion.oauth?.clientSecret, mcpSecretMarker('oauth')); + assert.equal(notion.oauth?.clientId, 'abc'); + // stdio server untouched + assert.ok(redacted.mcpServers.local && 'command' in redacted.mcpServers.local); + }); + + it('restores a sentinel from the previous on-disk value', () => { + const previous = withSecret('real-secret'); + const incoming = redactMcpConfigSecrets(previous); + const restored = restoreMcpConfigSecrets(incoming, previous); + const notion = restored.mcpServers.notion; + assert.ok(notion && 'url' in notion); + assert.equal(notion.oauth?.clientSecret, 'real-secret'); + }); + + it('rejects a sentinel when the endpoint URL changed', () => { + const previous = withSecret('real-secret'); + const incoming = redactMcpConfigSecrets(previous); + const notionIn = incoming.mcpServers.notion; + assert.ok(notionIn && 'url' in notionIn); + // A semi-trusted renderer keeps the sentinel but points the id at its + // own endpoint; main must not forward the unreadable secret there. + notionIn.url = 'https://attacker.example/mcp'; + assert.throws(() => restoreMcpConfigSecrets(incoming, previous), McpSecretRestoreError); + }); + + it('rejects a sentinel when the clientId changed', () => { + const previous = withSecret('real-secret'); + const incoming = redactMcpConfigSecrets(previous); + const notionIn = incoming.mcpServers.notion; + assert.ok(notionIn && 'url' in notionIn && notionIn.oauth); + notionIn.oauth.clientId = 'other-client'; + assert.throws(() => restoreMcpConfigSecrets(incoming, previous), McpSecretRestoreError); + }); + + it('rejects a sentinel that has no previous value instead of persisting or dropping it', () => { + const incoming = withSecret(mcpSecretMarker('oauth')); + assert.throws( + () => restoreMcpConfigSecrets(incoming, { version: MCP_CONFIG_VERSION, mcpServers: {} }), + McpSecretRestoreError, + ); + }); + + it('keeps a genuinely new secret (not the sentinel) on single-server restore', () => { + const config = withSecret('brand-new'); + const server = config.mcpServers.notion; + assert.ok(server); + const restored = restoreMcpServerSecret('notion', server, { version: MCP_CONFIG_VERSION, mcpServers: {} }); + assert.ok('url' in restored); + assert.equal(restored.oauth?.clientSecret, 'brand-new'); + }); +}); diff --git a/apps/desktop/src/main/mcp-ipc-main.ts b/apps/desktop/src/main/mcp-ipc-main.ts index 2fca1de8c4..226e1657bc 100644 --- a/apps/desktop/src/main/mcp-ipc-main.ts +++ b/apps/desktop/src/main/mcp-ipc-main.ts @@ -2,6 +2,11 @@ import type { IpcMain } from 'electron'; import type { McpConfigFile, McpServerConfig, McpServerStatus } from '@maka/core/mcp'; import type { McpClientManager } from '@maka/mcp'; import type { McpConfigStore } from '@maka/storage'; +import { + redactMcpConfigSecrets, + restoreMcpConfigSecrets, + restoreMcpServerSecret, +} from './mcp-secret-guard.js'; export interface McpIpcMainDeps { ipcMain: Pick; @@ -15,25 +20,41 @@ export interface McpIpcMainDeps { export function registerMcpIpcMain(deps: McpIpcMainDeps): void { const installs = new Map; settle(): void }>(); + // 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 + // and the manager (which needs the real secret) see it. deps.ipcMain.handle('mcp:getConfig', async () => { await deps.ensureReady(); - return deps.store.get(); + return redactMcpConfigSecrets(await deps.store.get()); }); deps.ipcMain.handle('mcp:listStatuses', async () => { 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.set(config); + const next = await deps.store.transform((current) => + restoreMcpConfigSecrets(config, current), + ); await deps.manager.sync(next); changed(deps); - return next; + return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:upsert', async (_event, serverId: string, config: McpServerConfig) => { - const next = await deps.store.upsert(serverId, config); + const next = await deps.store.transform((current) => ({ + ...current, + mcpServers: { + ...current.mcpServers, + [serverId]: restoreMcpServerSecret(serverId, config, current), + }, + })); await deps.manager.sync(next); changed(deps); - return next; + return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:install', async (_event, serverId: string, config: McpServerConfig) => { if (installs.has(serverId)) throw new Error(`MCP install already in progress: ${serverId}`); @@ -45,15 +66,21 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { }; installs.set(serverId, operation); try { - const next = await deps.store.upsert(serverId, config); - if (operation.cancelled) return next; + const next = await deps.store.transform((current) => ({ + ...current, + mcpServers: { + ...current.mcpServers, + [serverId]: restoreMcpServerSecret(serverId, config, current), + }, + })); + if (operation.cancelled) return redactMcpConfigSecrets(next); try { await deps.manager.sync(next); } catch (error) { if (!operation.cancelled) throw error; } if (!operation.cancelled) changed(deps); - return next; + return redactMcpConfigSecrets(next); } finally { if (installs.get(serverId) === operation) installs.delete(serverId); operation.settle(); @@ -63,7 +90,9 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { const next = await deps.store.remove(serverId); await deps.manager.sync(next); changed(deps); - return next; + // Still a full config crossing toward the renderer: the remaining + // servers' secrets must leave as sentinels here too. + return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:cancelInstall', async (_event, serverId: string) => { const operation = installs.get(serverId); @@ -73,7 +102,7 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void { const next = await deps.store.remove(serverId); await deps.manager.sync(next); changed(deps); - return next; + return redactMcpConfigSecrets(next); }); deps.ipcMain.handle('mcp:test', async (_event, serverId: string) => { await deps.ensureReady(); diff --git a/apps/desktop/src/main/mcp-secret-guard.ts b/apps/desktop/src/main/mcp-secret-guard.ts new file mode 100644 index 0000000000..3e6b0d09ea --- /dev/null +++ b/apps/desktop/src/main/mcp-secret-guard.ts @@ -0,0 +1,432 @@ +// apps/desktop/src/main/mcp-secret-guard.ts +// +// mcp.json carries credential material in several positions: a static +// OAuth clientSecret, remote request headers, remote URL query parameters +// (?api_key=…), stdio env values, and command-line arguments (--token …). +// The renderer is semi-trusted (SECURITY.md §3), so mcp:getConfig must not +// hand any of it over raw. Config leaves main with each such value +// replaced by a redaction marker; when the renderer sends an edited config +// back, markers are restored from what is on disk. +// +// The marker is occurrence-bound and non-confusable: +// - a per-process random nonce makes it impossible for a legitimate config +// value to collide with it (a fixed public sentinel is itself a valid +// string a user could enter); +// - a position tag (env key, header name, argument index, query key plus +// occurrence index, oauth) binds each marker to the exact place it was +// stamped, so a marker moved to another position — or a stale one from a +// previous process — REJECTS the write instead of acting as a wildcard. +// +// Which positions are masked: +// - headers: every value — the field exists to carry credentials. +// - env: every value — key-name heuristics miss real secrets (PGPASSWORD, +// DATABASE_URL, GITHUB_PAT), and the boundary is absolute; ordinary +// values are cheap for the user to retype. +// - args / URL query: values a sensitive-named flag or parameter +// introduces, plus anything the pattern redactor recognizes — free-form +// positions where full masking would destroy the editor. Repeated query +// keys mask and restore per occurrence, in order. +// +// Every restore is bound to the launch basis the value was configured +// for. A renderer that kept a marker but changed where the value flows — +// the URL for remote credentials; the command, arguments, cwd or any +// other env entry for stdio (each of those can redirect which executable +// runs or where it connects) — must not have main forward a secret it +// cannot read. An unrestorable marker REJECTS the write: silently dropping +// it would destroy stored values (and could leave an introducing flag +// consuming its neighbour) without the user ever learning why. The +// rejection message names the position, never the value. + +import { randomBytes } from 'node:crypto'; +import { + listMcpSecretLocations, + parseCommandFlag, + type McpSecretLocation, +} from '@maka/core/mcp-secrets'; +import { + isMcpStdioConfig, + MCP_CONFIG_VERSION, + type McpConfigFile, + type McpRemoteServerConfig, + type McpServerConfig, + type McpStdioServerConfig, +} from '@maka/core/mcp'; + +const MARKER_PREFIX = '__MAKA_MCP_SECRET_'; +const MARKER_NONCE = randomBytes(6).toString('base64url'); + +/** The redaction marker for one exact position. Tests build expected + * values with it; the renderer treats it as an opaque kept-value token. */ +export function mcpSecretMarker(tag: string): string { + return `${MARKER_PREFIX}${MARKER_NONCE}.${tag}__`; +} + +/** Whether a value is shaped like a redaction marker at all — including a + * displaced, hand-altered or previous-process one, which restore rejects. */ +export function isMcpSecretMarkerLike(value: string): boolean { + return value.startsWith(MARKER_PREFIX); +} + +/** Thrown when an incoming config keeps a marker whose stored value can no + * longer be restored — the launch basis changed, the marker was moved or is + * stale, or there is nothing on disk behind it. The write is rejected so + * the renderer can tell the user to re-enter the secret; the message never + * carries a value. */ +export class McpSecretRestoreError extends Error { + constructor(serverId: string, position: string) { + super( + `MCP server "${serverId}": the masked ${position} cannot be restored because its ` + + 'configuration basis changed — re-enter the secret value', + ); + this.name = 'McpSecretRestoreError'; + } +} + +/** Replaces every stored credential value with its position-bound marker + * for the renderer. */ +export function redactMcpConfigSecrets(config: McpConfigFile): McpConfigFile { + return mapServers(config, (server) => { + if (isMcpStdioConfig(server)) return redactStdio(server); + return redactRemote(server); + }); +} + +/** Restores markers in an incoming config from the on-disk previous + * config, so the renderer never has to hold the real values. */ +export function restoreMcpConfigSecrets( + incoming: McpConfigFile, + previous: McpConfigFile, +): McpConfigFile { + return mapServers(incoming, (server, serverId) => { + const prior = Object.hasOwn(previous.mcpServers, serverId) + ? previous.mcpServers[serverId] + : undefined; + if (isMcpStdioConfig(server)) return restoreStdio(serverId, server, prior); + return restoreRemote(serverId, server, prior); + }); +} + +/** Restores markers for a single upsert/insert config value. */ +export function restoreMcpServerSecret( + serverId: string, + config: McpServerConfig, + previous: McpConfigFile, +): McpServerConfig { + const restored = restoreMcpConfigSecrets( + { version: MCP_CONFIG_VERSION, mcpServers: { [serverId]: config } }, + previous, + ); + return restored.mcpServers[serverId] ?? config; +} + +// --------------------------------------------------------------------------- +// stdio + +function redactStdio(server: McpStdioServerConfig): McpServerConfig { + const locations = listMcpSecretLocations(server); + const next: McpStdioServerConfig = { ...server }; + if (next.env) { + // Every env value is masked regardless of the plan's credential split: + // the plan's `credential` flag governs scrub aggressiveness, while the + // IPC boundary is absolute for the whole map. + next.env = mapValues(next.env, (key, value) => + value ? mcpSecretMarker(`env.${key}`) : value, + ); + } + if (next.args) { + const argLocations = locations.filter( + (location): location is McpSecretLocation & { index: number } => + (location.kind === 'arg' || location.kind === 'arg-flag-value') && + location.index !== undefined, + ); + if (argLocations.length > 0) { + const args = [...next.args]; + for (const location of argLocations) { + args[location.index] = + location.kind === 'arg-flag-value' + ? `${location.key}=${mcpSecretMarker(`argflag.${location.index}`)}` + : mcpSecretMarker(`arg.${location.index}`); + } + next.args = args; + } + } + return next; +} + +function restoreStdio( + serverId: string, + server: McpStdioServerConfig, + prior: McpServerConfig | undefined, +): McpServerConfig { + const priorStdio = prior && isMcpStdioConfig(prior) ? prior : undefined; + const args = server.args ?? []; + const priorArgs = priorStdio?.args ?? []; + // The whole effective launch basis must match: command, cwd, the arg + // list shape, the env key set, and every value the renderer DID rewrite. + // Any deviation — a changed cwd, a new PATH, an extra flag — can point + // the restored secrets at a different executable or destination, so no + // marker may restore; the write rejects and the user re-enters the + // secrets (or reverts the basis change). + const sameBasis = + priorStdio !== undefined && + priorStdio.command === server.command && + (priorStdio.cwd ?? '') === (server.cwd ?? '') && + args.length === priorArgs.length && + args.every((arg, index) => argMatchesPrior(arg, index, priorArgs[index] ?? '')) && + sameKeySet(server.env ?? {}, priorStdio.env ?? {}) && + Object.entries(server.env ?? {}).every( + ([key, value]) => value === mcpSecretMarker(`env.${key}`) || priorStdio.env?.[key] === value, + ); + + const next: McpStdioServerConfig = { ...server }; + if (next.env) { + const env: Record = {}; + for (const [key, value] of Object.entries(next.env)) { + if (value === mcpSecretMarker(`env.${key}`)) { + const priorValue = sameBasis ? priorStdio.env?.[key] : undefined; + if (priorValue === undefined) { + throw new McpSecretRestoreError(serverId, `environment value "${key}"`); + } + env[key] = priorValue; + continue; + } + if (isMcpSecretMarkerLike(value)) { + // Displaced, altered or stale marker: never a wildcard. + throw new McpSecretRestoreError(serverId, `environment value "${key}"`); + } + env[key] = value; + } + next.env = env; + } + if (next.args) { + next.args = next.args.map((arg, index) => { + const restored = restoreArg(arg, index, sameBasis ? priorArgs[index] : undefined); + if (restored === undefined) { + // Dropping instead would leave the introducing flag consuming its + // neighbour (`--token --verbose`); rejection keeps the command + // line intact and the failure visible. + throw new McpSecretRestoreError(serverId, `argument ${index + 1}`); + } + return restored; + }); + } + return next; +} + +function argMatchesPrior(arg: string, index: number, priorArg: string): boolean { + if (arg === mcpSecretMarker(`arg.${index}`)) return true; + const flag = parseCommandFlag(arg); + if (flag?.value === mcpSecretMarker(`argflag.${index}`)) { + return priorArg.startsWith(`${flag.raw}=`); + } + return arg === priorArg; +} + +function restoreArg(arg: string, index: number, priorArg: string | undefined): string | undefined { + if (arg === mcpSecretMarker(`arg.${index}`)) return priorArg; + const flag = parseCommandFlag(arg); + if (flag?.value === mcpSecretMarker(`argflag.${index}`)) { + return priorArg?.startsWith(`${flag.raw}=`) ? priorArg : undefined; + } + // Any other marker-shaped argument (moved, altered, stale) is + // unrestorable by definition. + if (isMcpSecretMarkerLike(arg) || (flag?.value !== undefined && isMcpSecretMarkerLike(flag.value))) { + return undefined; + } + return arg; +} + +// --------------------------------------------------------------------------- +// remote + +function redactRemote(server: McpRemoteServerConfig): McpServerConfig { + const locations = listMcpSecretLocations(server); + const next: McpRemoteServerConfig = { ...server }; + next.url = maskUrlQuerySecrets(next.url, locations); + if (next.headers) { + next.headers = mapValues(next.headers, (key, value) => + value ? mcpSecretMarker(`header.${key}`) : value, + ); + } + if (next.oauth?.clientSecret) { + next.oauth = { ...next.oauth, clientSecret: mcpSecretMarker('oauth') }; + } + return next; +} + +function restoreRemote( + serverId: string, + server: McpRemoteServerConfig, + prior: McpServerConfig | undefined, +): McpServerConfig { + const priorRemote = prior && !isMcpStdioConfig(prior) ? prior : undefined; + const next: McpRemoteServerConfig = { ...server }; + next.url = restoreUrlQuerySecrets(serverId, next.url, priorRemote?.url); + // Both sides parsed: restoreUrlQuerySecrets returns a WHATWG-normalized + // string, so comparing against the raw stored URL would report a changed + // endpoint for a merely unnormalized one (missing path slash) and reject + // restores the user never invalidated. + const sameEndpoint = + priorRemote !== undefined && normalizeUrl(priorRemote.url) === normalizeUrl(next.url); + if (next.headers) { + const headers: Record = {}; + for (const [key, value] of Object.entries(next.headers)) { + if (value === mcpSecretMarker(`header.${key}`)) { + const priorValue = sameEndpoint ? priorRemote.headers?.[key] : undefined; + if (priorValue === undefined) { + throw new McpSecretRestoreError(serverId, `header "${key}"`); + } + headers[key] = priorValue; + continue; + } + if (isMcpSecretMarkerLike(value)) { + throw new McpSecretRestoreError(serverId, `header "${key}"`); + } + headers[key] = value; + } + next.headers = headers; + } + if (next.oauth?.clientSecret !== undefined) { + if (next.oauth.clientSecret === mcpSecretMarker('oauth')) { + const priorSecret = + sameEndpoint && priorRemote.oauth?.clientId === next.oauth.clientId + ? priorRemote.oauth?.clientSecret + : undefined; + if (priorSecret === undefined) { + throw new McpSecretRestoreError(serverId, 'oauth clientSecret'); + } + next.oauth = { ...next.oauth, clientSecret: priorSecret }; + } else if (isMcpSecretMarkerLike(next.oauth.clientSecret)) { + throw new McpSecretRestoreError(serverId, 'oauth clientSecret'); + } + } + return next; +} + +/** Masks query parameter values that carry credentials — a sensitive-named + * key (?api_key=…) or a pattern-recognized secret value. Repeated keys are + * masked per occurrence, in order, so `?token=a&token=b` round-trips both + * values instead of collapsing to one. */ +function maskUrlQuerySecrets(url: string, locations: readonly McpSecretLocation[]): string { + const parsed = parseUrl(url); + if (!parsed) return url; + const queryKeys = new Set( + locations + .filter((location) => location.kind === 'url-query') + .map((location) => location.key) + .filter((key): key is string => key !== undefined), + ); + if (queryKeys.size === 0) return url; + const occurrences = new Map(); + const entries = [...parsed.searchParams.entries()].map(([key, value]) => { + const occurrence = occurrences.get(key) ?? 0; + occurrences.set(key, occurrence + 1); + if (!queryKeys.has(key) || !value) return [key, value] as const; + return [key, mcpSecretMarker(`query.${key}.${occurrence}`)] as const; + }); + parsed.search = buildSearch(entries); + return parsed.toString(); +} + +/** Restores per-occurrence query markers from the prior URL — only when the + * URL is otherwise identical position by position (origin, path, key order, + * every unmasked value), so a repointed URL cannot inherit the secret. An + * unrestorable or displaced marker rejects the write. */ +function restoreUrlQuerySecrets(serverId: string, url: string, priorUrl: string | undefined): string { + const parsed = parseUrl(url); + if (!parsed) return url; + const entries = [...parsed.searchParams.entries()]; + const occurrences = new Map(); + const annotated = entries.map(([key, value]) => { + const occurrence = occurrences.get(key) ?? 0; + occurrences.set(key, occurrence + 1); + const masked = value === mcpSecretMarker(`query.${key}.${occurrence}`); + if (!masked && isMcpSecretMarkerLike(value)) { + throw new McpSecretRestoreError(serverId, `URL query parameter "${key}"`); + } + return { key, value, occurrence, masked }; + }); + if (!annotated.some((entry) => entry.masked)) return parsed.toString(); + + const prior = priorUrl ? parseUrl(priorUrl) : undefined; + const priorEntries = prior ? [...prior.searchParams.entries()] : []; + const sameRest = + prior !== undefined && + prior.origin === parsed.origin && + prior.pathname === parsed.pathname && + prior.hash === parsed.hash && + priorEntries.length === annotated.length && + annotated.every( + (entry, index) => + priorEntries[index]?.[0] === entry.key && + (entry.masked || priorEntries[index]?.[1] === entry.value), + ); + const restored = annotated.map((entry, index) => { + if (!entry.masked) return [entry.key, entry.value] as const; + const priorValue = sameRest ? priorEntries[index]?.[1] : undefined; + if (priorValue === undefined) { + throw new McpSecretRestoreError(serverId, `URL query parameter "${entry.key}"`); + } + return [entry.key, priorValue] as const; + }); + parsed.search = buildSearch(restored); + return parsed.toString(); +} + +/** Serializes entries preserving order and duplicates. */ +function buildSearch(entries: readonly (readonly [string, string])[]): string { + const params = new URLSearchParams(); + for (const [key, value] of entries) params.append(key, value); + return params.toString(); +} + +/** Canonical form for endpoint identity comparison; an unparsable URL falls + * back to its raw text. The query is re-serialized through the SAME + * serializer restore uses (URLSearchParams append), so a stored URL whose + * raw encoding differs only cosmetically (%20 vs +) is not misclassified as + * a changed endpoint — which would reject restores the user never + * invalidated. */ +function normalizeUrl(value: string): string { + const parsed = parseUrl(value); + if (!parsed) return value; + parsed.search = buildSearch([...parsed.searchParams.entries()]); + return parsed.toString(); +} + +// --------------------------------------------------------------------------- + +function sameKeySet(a: Record, b: Record): boolean { + const aKeys = Object.keys(a).sort(); + const bKeys = Object.keys(b).sort(); + return aKeys.length === bKeys.length && aKeys.every((key, index) => key === bKeys[index]); +} + +function parseUrl(value: string): URL | undefined { + try { + return new URL(value); + } catch { + return undefined; + } +} + +function mapValues( + record: Record, + transform: (key: string, value: string) => string, +): Record { + const result: Record = {}; + for (const [key, value] of Object.entries(record)) { + result[key] = transform(key, value); + } + return result; +} + +function mapServers( + config: McpConfigFile, + transform: (server: McpServerConfig, serverId: string) => McpServerConfig, +): McpConfigFile { + const mcpServers: Record = {}; + for (const [serverId, server] of Object.entries(config.mcpServers)) { + mcpServers[serverId] = transform(server, serverId); + } + return { ...config, mcpServers }; +} diff --git a/apps/desktop/src/renderer/mcp-page-model.ts b/apps/desktop/src/renderer/mcp-page-model.ts index bc04dd10f3..2cf6901378 100644 --- a/apps/desktop/src/renderer/mcp-page-model.ts +++ b/apps/desktop/src/renderer/mcp-page-model.ts @@ -1,4 +1,5 @@ import type { + McpOAuthConfig, McpProtocolPreference, McpServerConfig, McpServerStatus, @@ -18,6 +19,10 @@ export type McpEditorDraft = { transport: 'auto' | 'streamable-http' | 'sse'; protocol: McpProtocolPreference; headers: string; + /** Opaque round-trip state: the editor has no OAuth fields, but an + * edit → save of an OAuth-configured server must not delete the block + * (the masked clientSecret sentinel restores from disk in main). */ + oauth?: McpOAuthConfig; }; export function createEmptyMcpDraft(): McpEditorDraft { @@ -57,6 +62,7 @@ export function mcpDraftFromConfig(id: string, config: McpServerConfig): McpEdit // not the default for a newly-authored remote entry. protocol: resolveMcpRemoteProtocolPreference(config), headers: formatMap(config.headers), + ...(config.oauth ? { oauth: config.oauth } : {}), }; } @@ -94,6 +100,7 @@ export function mcpConfigFromDraft(draft: McpEditorDraft, copy: McpCopy): McpSer transport: draft.transport, protocol: draft.transport === 'sse' ? 'legacy' : draft.protocol, headers: parseMap(draft.headers, copy), + ...(draft.oauth ? { oauth: draft.oauth } : {}), }; } diff --git a/apps/desktop/src/renderer/mcp-page.tsx b/apps/desktop/src/renderer/mcp-page.tsx index b9fba58a7f..60a1a73511 100644 --- a/apps/desktop/src/renderer/mcp-page.tsx +++ b/apps/desktop/src/renderer/mcp-page.tsx @@ -86,7 +86,7 @@ import { } from './mcp-page-model'; import { settingsActionErrorMessage } from './settings/settings-error-copy'; import { getMcpCopy, type McpCopy } from './locales/mcp-copy'; -import { formatCommandLine, parseCommandLine } from './mcp-command-line'; +import { formatCommandLine } from './mcp-command-line'; import { validateMcpEditorDraft, type McpEditorErrors, diff --git a/packages/core/package.json b/packages/core/package.json index ffc9eec2e1..9906819742 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -98,6 +98,7 @@ "./result": "./dist/result.js", "./settings": "./dist/settings.js", "./mcp": "./dist/mcp.js", + "./mcp-secrets": "./dist/mcp-secrets.js", "./settings/network-settings": "./dist/settings/network-settings.js", "./usage-stats/types": "./dist/usage-stats/types.js", "./usage-stats/pricing": "./dist/usage-stats/pricing.js", diff --git a/packages/core/src/__tests__/mcp-secrets.test.ts b/packages/core/src/__tests__/mcp-secrets.test.ts new file mode 100644 index 0000000000..d9f3b8b061 --- /dev/null +++ b/packages/core/src/__tests__/mcp-secrets.test.ts @@ -0,0 +1,51 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { + MCP_SECRET_WITHHELD_MESSAGE, + mcpSecretInventory, + scrubKnownMcpSecrets, +} from '../mcp-secrets.js'; + +describe('MCP secret inventory and scrubbing', () => { + it('substitutes longer secrets before their prefixes', () => { + // With both configured, replacing `abcd` first would shred `abcdEFGH` + // into `[redacted]EFGH` and leave its tail exposed. + const scrubbed = scrubKnownMcpSecrets('token abcdEFGH and abcd here', { + substitute: ['abcd', 'abcdEFGH'], + withhold: [], + }); + assert.equal(scrubbed, 'token [redacted] and [redacted] here'); + assert.doesNotMatch(scrubbed, /EFGH/u); + }); + + it('bounded-substitutes short fragments of a longer credential instead of withholding', () => { + // "Bearer x" splits into the one-character part "x": withholding on it + // would collapse every message containing that letter, but ignoring it + // would leak an upstream that reflects the bare fragment. It is + // substituted at token boundaries only. + const inventory = mcpSecretInventory({ + url: 'https://api.example.com/mcp', + headers: { Authorization: 'Bearer x' }, + }); + assert.deepEqual(inventory.withhold, []); + assert.deepEqual(inventory.substituteBounded, ['x']); + const scrubbed = scrubKnownMcpSecrets('an x marks the spot; Bearer x leaked', inventory); + assert.notEqual(scrubbed, MCP_SECRET_WITHHELD_MESSAGE); + assert.doesNotMatch(scrubbed, /Bearer x/u); + // The standalone fragment is gone; words merely containing the + // character are untouched. + assert.equal(scrubbed, 'an [redacted] marks the spot; [redacted] leaked'); + assert.equal( + scrubKnownMcpSecrets('token x expired for example.com', inventory), + 'token [redacted] expired for example.com', + ); + + // A whole credential VALUE too short to splice still withholds. + const short = mcpSecretInventory({ + url: 'https://api.example.com/mcp', + headers: { Authorization: 'k7#' }, + }); + assert.deepEqual(short.withhold, ['k7#']); + assert.equal(scrubKnownMcpSecrets('echoing k7# back', short), MCP_SECRET_WITHHELD_MESSAGE); + }); +}); diff --git a/packages/core/src/mcp-secrets.ts b/packages/core/src/mcp-secrets.ts new file mode 100644 index 0000000000..2a6a84c38e --- /dev/null +++ b/packages/core/src/mcp-secrets.ts @@ -0,0 +1,268 @@ +// packages/core/src/mcp-secrets.ts +// +// The single authority for WHERE credential material lives in an MCP server +// config and WHICH values must never leave the main process in cleartext. +// Two consumers derive from the same plan so they cannot drift: +// +// - the desktop IPC boundary (mcp-secret-guard) masks exactly these +// locations before a config crosses toward the renderer; +// - the runtime scrubber (packages/mcp) collects exactly these values to +// scrub every outbound error, status, stderr and tool payload. +// +// The plan enumerates locations; masking mechanics (sentinels, restore +// binding) stay with the IPC guard, and scrubbing mechanics (substitution, +// withholding) stay here as shared primitives. + +import { isSensitiveKey, redactSecrets } from './redaction.js'; +import { isMcpStdioConfig, type McpServerConfig } from './mcp.js'; + +/** One credential-bearing position in a server config. */ +export interface McpSecretLocation { + /** Structural position — the IPC guard masks by this. */ + kind: 'header' | 'oauth-client-secret' | 'env' | 'arg' | 'arg-flag-value' | 'url-query'; + /** Header name / env key / query parameter, where applicable. */ + key?: string; + /** Argument index, for arg kinds. */ + index?: number; + /** The secret value itself. */ + value: string; + /** + * Credential-position values (headers, clientSecret, sensitive-keyed env, + * flagged args, sensitive query params) are protected at any length; a + * plain env value is ordinary configuration unless long enough to be an + * unambiguous match. + */ + credential: boolean; +} + +/** Below this length a value cannot be spliced out of prose without + * shredding it (replacing every "ab" would mangle ordinary messages), and a + * plain configuration value this short is not a usable secret either. */ +export const MCP_SECRET_MIN_SUBSTITUTION_LENGTH = 4; + +export const MCP_SECRET_WITHHELD_MESSAGE = + 'message withheld: the upstream text contained credential material'; + +/** What the scrubber does with each value: long-enough values are + * substituted in place; a WHOLE credential too short to splice out forces + * the containing message to be withheld; a short FRAGMENT of a longer + * credential ("Bearer x" → "x") is substituted only at token boundaries — + * an upstream reflecting the bare fragment still loses it, without every + * message containing that character collapsing. */ +export interface McpSecretInventory { + substitute: string[]; + /** Short credential fragments, replaced only as standalone tokens. */ + substituteBounded?: string[]; + withhold: string[]; +} + +export const EMPTY_MCP_SECRET_INVENTORY: McpSecretInventory = Object.freeze({ + substitute: [], + substituteBounded: [], + withhold: [], +}); + +/** Whether a command-line flag introduces a credential value (`--token x`, + * `--api-key=x`). Shared so the guard's masking and any future consumer + * agree on what counts. */ +export function isSensitiveFlagName(name: string): boolean { + return isSensitiveKey(name); +} + +/** Whether a standalone value looks like a secret to the pattern redactor — + * the only signal available for free-form positions (args, query values). */ +export function isPatternSecret(value: string): boolean { + return value.length > 0 && redactSecrets(value) !== value; +} + +const FLAG_PATTERN = /^(--?[A-Za-z][\w-]*)(?:=([\s\S]*))?$/u; + +export function parseCommandFlag( + arg: string, +): { raw: string; name: string; value?: string } | undefined { + const match = FLAG_PATTERN.exec(arg); + if (!match) return undefined; + const raw = match[1] ?? arg; + return { + raw, + name: raw.replace(/^--?/u, ''), + ...(match[2] !== undefined ? { value: match[2] } : {}), + }; +} + +/** Enumerates every credential-bearing location in a server config — the + * shared secret-location plan. */ +export function listMcpSecretLocations(config: McpServerConfig): McpSecretLocation[] { + const locations: McpSecretLocation[] = []; + if (isMcpStdioConfig(config)) { + for (const [key, value] of Object.entries(config.env ?? {})) { + if (!value) continue; + // Every env value is masked at the IPC boundary (key-name heuristics + // miss real secrets — PGPASSWORD, DATABASE_URL); only sensitive-keyed + // values are credential-position for scrubbing, because withholding + // messages over NODE_ENV=1 would swallow nearly everything. + locations.push({ kind: 'env', key, value, credential: isSensitiveKey(key) }); + } + const args = config.args ?? []; + for (let index = 0; index < args.length; index += 1) { + const arg = args[index] ?? ''; + const previous = index > 0 ? (args[index - 1] ?? '') : ''; + const previousFlag = parseCommandFlag(previous); + const flag = parseCommandFlag(arg); + if ( + previousFlag && + previousFlag.value === undefined && + isSensitiveFlagName(previousFlag.name) && + // `--token --verbose` is a flag pair, not a flag and its secret: + // classifying the second flag as credential material would hide it + // from the editor and change the launched command. + !flag + ) { + locations.push({ kind: 'arg', index, value: arg, credential: true }); + continue; + } + // A sensitive flag NAME marks its value; but a value that itself looks + // like a token is a secret behind any name — `--custom=sk-…` must not + // slip through because "custom" sounds harmless. The flag name is an + // additional signal, never an exclusion. + if ( + flag?.value !== undefined && + (isSensitiveFlagName(flag.name) || isPatternSecret(flag.value)) + ) { + locations.push({ + kind: 'arg-flag-value', + index, + key: flag.raw, + value: flag.value, + credential: true, + }); + continue; + } + if (!flag && isPatternSecret(arg)) { + locations.push({ kind: 'arg', index, value: arg, credential: true }); + } + } + return locations; + } + for (const [key, value] of Object.entries(config.headers ?? {})) { + if (!value) continue; + // The headers field exists to carry credentials: every value counts. + locations.push({ kind: 'header', key, value, credential: true }); + } + if (config.oauth?.clientSecret) { + locations.push({ + kind: 'oauth-client-secret', + value: config.oauth.clientSecret, + credential: true, + }); + } + const url = parseUrl(config.url); + if (url) { + for (const [key, value] of url.searchParams.entries()) { + if (value && (isSensitiveKey(key) || isPatternSecret(value))) { + locations.push({ kind: 'url-query', key, value, credential: true }); + } + } + } + return locations; +} + +/** Builds the scrub inventory from the shared location plan. A whole value + * short enough to be un-spliceable forces withholding when it sits in a + * credential position; whitespace-separated PARTS of a longer value are + * substituted when long enough (a reflected token loses its "Bearer" prefix + * and still matches) but never withheld — a one-character part like the "x" + * of "Bearer x" would otherwise collapse every message containing that + * letter, and the full-value substitution already covers realistic + * reflections. */ +export function mcpSecretInventory(config: McpServerConfig): McpSecretInventory { + const inventory: McpSecretInventory = { substitute: [], substituteBounded: [], withhold: [] }; + for (const location of listMcpSecretLocations(config)) { + const value = location.value; + if (value.length === 0) continue; + if (value.length >= MCP_SECRET_MIN_SUBSTITUTION_LENGTH) { + inventory.substitute.push(value); + } else if (location.credential) { + inventory.withhold.push(value); + } + for (const part of value.split(/\s+/u)) { + if (part === value || part.length === 0) continue; + if (part.length >= MCP_SECRET_MIN_SUBSTITUTION_LENGTH) { + inventory.substitute.push(part); + } else if (location.credential) { + // The short tail of "Bearer x": an upstream reflecting the bare + // fragment must still lose it, at token boundaries only. + inventory.substituteBounded?.push(part); + } + } + } + return inventory; +} + +/** Substitutes every known long secret and withholds the whole message when + * an un-spliceable short credential appears in it. Longer secrets are + * replaced first: with `abcd` and `abcdEFGH` both configured, processing + * `abcd` first would shred `abcdEFGH` into `[redacted]EFGH` and leave its + * tail exposed. */ +export function scrubKnownMcpSecrets(message: string, secrets: McpSecretInventory): string { + // Withholding is checked on the RAW message: substitution first could + // consume a short credential embedded in a longer secret ("Bearer ab") + // and let the message through as an in-place redaction, violating the + // invariant that any message containing an un-spliceable credential is + // withheld wholesale. + if (secrets.withhold.some((secret) => message.includes(secret))) { + return MCP_SECRET_WITHHELD_MESSAGE; + } + let result = message; + for (const secret of [...new Set(secrets.substitute)].sort((a, b) => b.length - a.length)) { + result = result.split(secret).join('[redacted]'); + } + for (const fragment of new Set(secrets.substituteBounded ?? [])) { + result = result.replace(boundedFragmentPattern(fragment), '[redacted]'); + } + return result; +} + +/** Matches a short credential fragment only as a standalone token, so + * scrubbing "x" redacts `token x expired` without shredding "example". */ +function boundedFragmentPattern(fragment: string): RegExp { + const escaped = fragment.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&'); + return new RegExp(`(?(value: T, secrets: McpSecretInventory): T { + if ( + secrets.substitute.length === 0 && + secrets.withhold.length === 0 && + (secrets.substituteBounded?.length ?? 0) === 0 + ) { + return value; + } + if (typeof value === 'string') return scrubKnownMcpSecrets(value, secrets) as T; + if (Array.isArray(value)) return value.map((item) => deepScrubMcpSecrets(item, secrets)) as T; + if (isPlainRecord(value)) { + return Object.fromEntries( + Object.entries(value).map(([key, item]) => [ + scrubKnownMcpSecrets(key, secrets), + deepScrubMcpSecrets(item, secrets), + ]), + ) as T; + } + return value; +} + +function parseUrl(value: string): URL | undefined { + try { + return new URL(value); + } catch { + return undefined; + } +} + +function isPlainRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} diff --git a/packages/core/src/mcp.ts b/packages/core/src/mcp.ts index 3070dbe431..c15558966c 100644 --- a/packages/core/src/mcp.ts +++ b/packages/core/src/mcp.ts @@ -18,6 +18,21 @@ export interface McpRemoteServerConfig { transport?: 'streamable-http' | 'sse' | 'auto'; headers?: Record; protocol?: McpProtocolPreference; + oauth?: McpOAuthConfig; +} + +/** + * Static OAuth client settings for servers whose authorization server does + * not support dynamic registration (RFC 7591) or CIMD. All fields are + * optional: with none set, the client registers dynamically and listens on + * an ephemeral loopback port. A pre-registered client usually pins + * `callbackPort`, because its redirect URI was registered with a fixed port. + */ +export interface McpOAuthConfig { + clientId?: string; + clientSecret?: string; + scopes?: string[]; + callbackPort?: number; } export type McpServerConfig = McpStdioServerConfig | McpRemoteServerConfig; diff --git a/packages/core/src/redaction.ts b/packages/core/src/redaction.ts index f434bb320c..e6fc39705f 100644 --- a/packages/core/src/redaction.ts +++ b/packages/core/src/redaction.ts @@ -156,7 +156,12 @@ function redactUrlQuerySecrets(value: string): string { }); } -function isSensitiveKey(key: string): boolean { +/** Whether a key NAME marks its value as credential material (TOKEN, + * API_KEY, clientSecret, …). Exported for callers that must decide whether + * a keyed value is a secret — e.g. which MCP stdio env values are masked at + * the IPC boundary — so the heuristic cannot drift from the one redaction + * itself applies. */ +export function isSensitiveKey(key: string): boolean { const segments = sensitiveKeySegments(key); const suffix = segments.at(-1); if (!suffix) return false; diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index 36a150c707..731cd582d6 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -172,6 +172,27 @@ test('allows SSE only with an omitted or explicit legacy protocol', () => { } }); +test('transform sees the latest committed config, not a caller snapshot', async () => { + // The restore-plus-mutation seam: a marker-bearing write that derived its + // restores from a stale snapshot could roll a rotated secret back. Inside + // transform, apply() must observe the concurrent writer's commit. + const root = await tempRoot(); + const store = createMcpConfigStore(root); + await store.upsert('local', { command: 'npx', env: { TOKEN: 'v1' } }); + const rotate = store.upsert('local', { command: 'npx', env: { TOKEN: 'v2-rotated' } }); + const observed: string[] = []; + const restoreLike = store.transform((current) => { + const server = current.mcpServers.local; + if (server && 'command' in server && server.env) observed.push(server.env.TOKEN ?? ''); + return current; + }); + await Promise.all([rotate, restoreLike]); + assert.deepEqual(observed, ['v2-rotated']); + const final = (await store.get()).mcpServers.local; + assert.ok(final && 'command' in final); + assert.equal(final.env?.TOKEN, 'v2-rotated'); +}); + test('serializes concurrent updates without corrupting the file', async () => { const root = await tempRoot(); const store = createMcpConfigStore(root); @@ -292,6 +313,85 @@ test('full replacement can migrate an existing version 1 wrapper to version 2', }); }); +test('normalizes and bounds the remote oauth block', async () => { + const normalized = normalizeMcpConfig({ + version: 1, + mcpServers: { + notion: { + url: 'https://mcp.notion.com/mcp', + oauth: { clientId: 'abc', scopes: ['read', 'write'], callbackPort: 33389 }, + }, + }, + }); + const notion = normalized.mcpServers.notion; + assert.ok(notion && 'url' in notion); + assert.deepEqual(notion.oauth, { + clientId: 'abc', + scopes: ['read', 'write'], + callbackPort: 33389, + }); + + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { bad: { url: 'https://example.com/mcp', oauth: { callbackPort: 0 } } }, + }), + /callbackPort/u, + ); + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { bad: { url: 'https://example.com/mcp', oauth: { clientId: '' } } }, + }), + /clientId/u, + ); + // A clientSecret alone cannot form static client credentials. + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { bad: { url: 'https://example.com/mcp', oauth: { clientSecret: 's3cr3t' } } }, + }), + /clientId is required/u, + ); + // stdio servers have no oauth block; unknown fields there stay rejected + // by the stdio branch simply dropping them. + const stdio = normalizeMcpConfig({ + version: 1, + mcpServers: { local: { command: 'npx', oauth: { clientId: 'x' } } }, + }).mcpServers.local; + assert.ok(stdio && !('oauth' in stdio)); +}); + +test('rejects a config that declares both an Authorization header and oauth', () => { + assert.throws( + () => + normalizeMcpConfig({ + version: 1, + mcpServers: { + bad: { + url: 'https://example.com/mcp', + headers: { authorization: 'Bearer x' }, + oauth: { clientId: 'abc' }, + }, + }, + }), + /must not include Authorization when oauth is configured/u, + ); + // Either alone is fine. + assert.doesNotThrow(() => + normalizeMcpConfig({ + version: 1, + mcpServers: { + headerOnly: { url: 'https://example.com/mcp', headers: { Authorization: 'Bearer x' } }, + oauthOnly: { url: 'https://example.com/mcp', oauth: { clientId: 'abc' } }, + }, + }), + ); +}); + async function tempRoot(): Promise { const root = await mkdtemp(join(tmpdir(), 'maka-mcp-store-')); roots.push(root); diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index 1e9b8d11dc..de9ce6222e 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -5,6 +5,7 @@ import { MCP_CONFIG_VERSION, createDefaultMcpConfig, type McpConfigFile, + type McpOAuthConfig, type McpProtocolPreference, type McpRemoteServerConfig, type McpServerConfig, @@ -20,6 +21,11 @@ const FORBIDDEN_KEYS = new Set(['__proto__', 'prototype', 'constructor']); export interface McpConfigStore { get(): Promise; set(config: McpConfigFile): Promise; + /** One serialized read-transform-write. `apply` sees the CURRENT on-disk + * config and returns the next one, inside the store's write queue — the + * seam for restore-plus-mutation flows whose separate get()-then-write + * would race a concurrent writer and roll a rotated secret back. */ + transform(apply: (current: McpConfigFile) => McpConfigFile): Promise; upsert(serverId: string, config: McpServerConfig): Promise; remove(serverId: string): Promise; } @@ -65,6 +71,15 @@ class FileMcpConfigStore implements McpConfigStore { }); } + async transform(apply: (current: McpConfigFile) => McpConfigFile): Promise { + return this.serial(async () => { + const current = await this.readOrCreate(); + const next = normalizeMcpConfig(apply(current)); + await this.write(next); + return next; + }); + } + async upsert(serverId: string, config: McpServerConfig): Promise { assertSafeKey(serverId, 'server id'); return this.serial(async () => { @@ -212,6 +227,47 @@ function normalizeServer( }; if (value.headers !== undefined) result.headers = stringMap(value.headers, `${serverId}.headers`); if (protocol !== undefined) result.protocol = protocol; + if (value.oauth !== undefined) result.oauth = normalizeOAuth(value.oauth, serverId); + if ( + result.oauth && + Object.keys(result.headers ?? {}).some((key) => key.toLowerCase() === 'authorization') + ) { + // One authority per header: the OAuth bearer owns Authorization. A + // config declaring both is a conflict to reject, not to arbitrate at + // request time. + throw new Error(`${serverId}.headers must not include Authorization when oauth is configured`); + } + return result; +} + +function normalizeOAuth(value: unknown, serverId: string): McpOAuthConfig { + if (!isRecord(value)) throw new Error(`${serverId}.oauth must be an object`); + const result: McpOAuthConfig = {}; + if (value.clientId !== undefined) { + result.clientId = nonEmptyString(value.clientId, `${serverId}.oauth.clientId`); + } + if (value.clientSecret !== undefined) { + result.clientSecret = nonEmptyString(value.clientSecret, `${serverId}.oauth.clientSecret`); + } + if (result.clientSecret !== undefined && result.clientId === undefined) { + // A secret with no client id cannot form static client credentials — + // authentication would fail later, far from the config mistake. + throw new Error(`${serverId}.oauth.clientId is required when clientSecret is configured`); + } + if (value.scopes !== undefined) { + result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`); + } + if (value.callbackPort !== undefined) { + if ( + typeof value.callbackPort !== 'number' || + !Number.isInteger(value.callbackPort) || + value.callbackPort < 1 || + value.callbackPort > 65_535 + ) { + throw new Error(`${serverId}.oauth.callbackPort must be a port number`); + } + result.callbackPort = value.callbackPort; + } return result; }