From ac82925bbb2c0976c51c721d86abf59c9476eff3 Mon Sep 17 00:00:00 2001 From: sathyapramod <8750601+sathyapramod@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:31:05 +0530 Subject: [PATCH 1/3] feat: implement self-connection guard for MCP endpoints Added a mechanism to prevent the daemon from connecting to its own MCP HTTP and gRPC endpoints. This includes: - Updated `McpClientPool` to track listening ports and reject self-referential connections. - Introduced `isSelfConnectionUrl` utility for URL validation against local addresses. - Enhanced tests to verify self-connection rejection and allow non-self remote MCP URLs. Additionally, updated documentation to reflect these changes and ensure clarity on the new self-connection behavior. --- docs/ARCHITECTURE.md | 4 + docs/decisions.md | 18 +++ package-lock.json | 2 +- packages/daemon/src/daemon/daemon.ts | 13 +- packages/daemon/src/daemon/index.ts | 2 +- .../daemon/src/daemon/mcp-client-pool.test.ts | 121 +++++++++++++++- packages/daemon/src/daemon/mcp-client-pool.ts | 135 ++++++++++++++++-- packages/daemon/src/daemon/state.ts | 4 +- packages/daemon/src/daemon/web/server.ts | 3 + .../integration/mcp-self-connection.test.ts | 126 ++++++++++++++++ 10 files changed, 409 insertions(+), 19 deletions(-) create mode 100644 packages/daemon/tests/integration/mcp-self-connection.test.ts diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3fcc958..1a6fabb 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -325,6 +325,10 @@ Manages connections to external MCP servers defined in config. Uses `@ai-sdk/mcp - Supports stdio and HTTP/SSE transports - Auto-discovers tools on connect and registers them in `ToolRegistry` - Hot-reloads when config changes (connects new, disconnects removed) +- Refuses self-connections: HTTP/SSE URLs that target this daemon's own + listening HTTP or gRPC ports on a local address (`localhost`, `127.0.0.1`, + `::1`, hostname, or any local interface IP) are rejected so the daemon + cannot recurse into its own `/mcp` endpoint ```yaml # In config.yaml diff --git a/docs/decisions.md b/docs/decisions.md index dff3513..302cdbf 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -407,3 +407,21 @@ fine for early adopters but a barrier to organic adoption. Publishing to both VS Code Marketplace and OpenVSX ensures coverage for VS Code and compatible editors (Eclipse Theia, VSCodium, Gitpod). Gating alpha releases prevents incomplete builds from reaching end users. + +--- + +## DR-029: Block MCP self-connections to the daemon's own endpoints + +**Date:** 2026-07-20 +**Decision:** `McpClientPool` tracks the daemon's HTTP and gRPC listen ports +via `setListenEndpoints()` and rejects HTTP/SSE MCP server URLs whose host is +a local address (loopback, hostname, or any local interface IP) and whose port +matches a tracked listen port. Both config `connect()` and dynamic +`connectDynamic()` throw a clear self-connection error. Listen ports are +registered at daemon start (`httpPort` / `grpcPort` options) and when the +embedded web server binds. +**Rationale:** `isSelfConnection()` previously always returned `false`, so the +daemon could register its own `/mcp` HTTP endpoint as an external MCP server +and amplify tool calls recursively. Comparing target URL host+port against +known listen endpoints closes that loop without blocking legitimate remote or +other-localhost MCP servers on different ports. diff --git a/package-lock.json b/package-lock.json index db65c7e..a186433 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11200,7 +11200,7 @@ "vitest": "^4.1.9" }, "engines": { - "node": ">=20.0.0" + "node": "^20.19.0 || >=22.12.0" } }, "packages/daemon/node_modules/@grpc/proto-loader": { diff --git a/packages/daemon/src/daemon/daemon.ts b/packages/daemon/src/daemon/daemon.ts index 76a4ccd..f3716e7 100644 --- a/packages/daemon/src/daemon/daemon.ts +++ b/packages/daemon/src/daemon/daemon.ts @@ -85,6 +85,11 @@ export interface DaemonOptions { grpcPort?: number; /** Host/IP to bind the TCP gRPC listener to (default: 127.0.0.1). */ grpcHost?: string; + /** + * Expected HTTP/web port for self-connection detection. + * Set even before the web server binds so MCP init cannot recurse into /mcp. + */ + httpPort?: number; } /** @@ -110,7 +115,13 @@ export async function startDaemon(opts?: DaemonOptions): Promise { // Create state state = new DaemonState(); - + + // Register known listen ports before MCP init so self-connections are blocked + state.mcpClientPool.setListenEndpoints({ + httpPorts: opts?.httpPort != null ? [opts.httpPort] : [], + grpcPorts: opts?.grpcPort != null ? [opts.grpcPort] : [], + }); + // Load proto and create server const proto = loadProto(); const abbenayProto = (proto as unknown as { abbenay: { v1: { Abbenay: { service: grpc.ServiceDefinition } } } }).abbenay.v1; diff --git a/packages/daemon/src/daemon/index.ts b/packages/daemon/src/daemon/index.ts index 69f09ed..e87ddd6 100644 --- a/packages/daemon/src/daemon/index.ts +++ b/packages/daemon/src/daemon/index.ts @@ -158,7 +158,7 @@ async function runServer(opts: ServerOptions): Promise { }); } else { console.log('No daemon running, starting in-process...'); - const daemonState = await startDaemon({ keepAlive: false, grpcPort, grpcHost }); + const daemonState = await startDaemon({ keepAlive: false, grpcPort, grpcHost, httpPort: port }); const { url, app } = await startEmbeddedWebServer(daemonState, port); if (mcp && app) { diff --git a/packages/daemon/src/daemon/mcp-client-pool.test.ts b/packages/daemon/src/daemon/mcp-client-pool.test.ts index 05a6b13..67f1f22 100644 --- a/packages/daemon/src/daemon/mcp-client-pool.test.ts +++ b/packages/daemon/src/daemon/mcp-client-pool.test.ts @@ -44,7 +44,8 @@ vi.mock('@ai-sdk/mcp/mcp-stdio', () => ({ // ── Import (after mocks) ────────────────────────────────────────────────── -import { McpClientPool } from './mcp-client-pool.js'; +import { McpClientPool, isSelfConnectionUrl, normalizeHost } from './mcp-client-pool.js'; +import * as os from 'node:os'; // ── Test setup ──────────────────────────────────────────────────────────── @@ -390,3 +391,121 @@ describe('callTool', () => { await expect(pool.callTool('mcp:empty', 'missing', {})).rejects.toThrow('not found'); }); }); + +// ── self-connection guard ───────────────────────────────────────────────── + +describe('isSelfConnectionUrl', () => { + const endpoints = { httpPorts: [8787], grpcPorts: [50051] }; + + it('detects localhost / 127.0.0.1 / IPv6 loopback on HTTP port', () => { + expect(isSelfConnectionUrl('http://localhost:8787/mcp', endpoints)).toBe(true); + expect(isSelfConnectionUrl('http://127.0.0.1:8787/mcp', endpoints)).toBe(true); + expect(isSelfConnectionUrl('http://[::1]:8787/mcp', endpoints)).toBe(true); + expect(isSelfConnectionUrl('HTTP://LocalHost:8787/MCP', endpoints)).toBe(true); + }); + + it('detects self on gRPC listen port', () => { + expect(isSelfConnectionUrl('http://127.0.0.1:50051/', endpoints)).toBe(true); + }); + + it('detects machine hostname when port matches', () => { + const host = normalizeHost(os.hostname()); + expect(isSelfConnectionUrl(`http://${host}:8787/mcp`, endpoints)).toBe(true); + }); + + it('allows remote hosts even when port number matches', () => { + expect(isSelfConnectionUrl('http://mcp.example.com:8787/mcp', endpoints)).toBe(false); + expect(isSelfConnectionUrl('https://remote.example.org/mcp', endpoints)).toBe(false); + }); + + it('allows localhost on a different port', () => { + expect(isSelfConnectionUrl('http://127.0.0.1:3001/sse', endpoints)).toBe(false); + expect(isSelfConnectionUrl('http://localhost:9999/mcp', endpoints)).toBe(false); + }); + + it('returns false when no listen ports are configured', () => { + expect(isSelfConnectionUrl('http://127.0.0.1:8787/mcp', { + httpPorts: [], + grpcPorts: [], + })).toBe(false); + }); + + it('returns false for invalid URLs', () => { + expect(isSelfConnectionUrl('not-a-url', endpoints)).toBe(false); + }); +}); + +describe('self-connection rejection', () => { + beforeEach(() => { + pool.setListenEndpoints({ httpPorts: [8787], grpcPorts: [] }); + }); + + it('rejects config connect to own /mcp (cannot remain a no-op)', async () => { + await expect(pool.connect('self', { + transport: 'http', + url: 'http://127.0.0.1:8787/mcp', + enabled: true, + })).rejects.toThrow(/self-connection/i); + + expect(pool.connectedCount).toBe(0); + expect(mockCreateMCPClient).not.toHaveBeenCalled(); + }); + + it('rejects all localhost URL variants for dynamic registration', async () => { + const variants = [ + 'http://localhost:8787/mcp', + 'http://127.0.0.1:8787/mcp', + 'http://[::1]:8787/mcp', + `http://${normalizeHost(os.hostname())}:8787/mcp`, + ]; + + for (const [i, url] of variants.entries()) { + await expect(pool.connectDynamic(`self-${i}`, { + transport: 'http', + url, + enabled: true, + })).rejects.toThrow(/self-connection/i); + } + + expect(mockCreateMCPClient).not.toHaveBeenCalled(); + }); + + it('rejects SSE transport self-connections', async () => { + await expect(pool.connectDynamic('self-sse', { + transport: 'sse', + url: 'http://localhost:8787/sse', + enabled: true, + })).rejects.toThrow(/self-connection/i); + }); + + it('allows non-self remote HTTP MCP URLs', async () => { + await pool.connectDynamic('remote', { + transport: 'http', + url: 'http://mcp.example.com:443/mcp', + enabled: true, + }); + + expect(pool.getStatus('remote')?.connected).toBe(true); + expect(mockCreateMCPClient).toHaveBeenCalled(); + }); + + it('allows localhost MCP on a different port', async () => { + await pool.connect('other-local', { + transport: 'http', + url: 'http://127.0.0.1:3001/sse', + enabled: true, + }); + + expect(pool.getStatus('other-local')?.connected).toBe(true); + }); + + it('skips self URLs during connectAll without aborting other servers', async () => { + await pool.connectAll({ + self: { transport: 'http', url: 'http://localhost:8787/mcp', enabled: true }, + ok: { transport: 'stdio', command: 'ok', enabled: true }, + }); + + expect(pool.getStatus('self')?.connected).not.toBe(true); + expect(pool.getStatus('ok')?.connected).toBe(true); + }); +}); diff --git a/packages/daemon/src/daemon/mcp-client-pool.ts b/packages/daemon/src/daemon/mcp-client-pool.ts index 14db92c..18ac4cf 100644 --- a/packages/daemon/src/daemon/mcp-client-pool.ts +++ b/packages/daemon/src/daemon/mcp-client-pool.ts @@ -7,6 +7,7 @@ * Uses @ai-sdk/mcp for the MCP client implementation. */ +import * as os from 'node:os'; import { createMCPClient, type MCPClient } from '@ai-sdk/mcp'; import { Experimental_StdioMCPTransport as StdioMCPTransport } from '@ai-sdk/mcp/mcp-stdio'; import type { ToolRegistry } from '../core/tool-registry.js'; @@ -32,6 +33,83 @@ export interface McpServerStatus { scope?: DynamicScope; } +/** + * Ports this daemon is listening on. Used to refuse MCP client connections + * that would recurse into our own HTTP/MCP or gRPC endpoints. + */ +export interface DaemonListenEndpoints { + httpPorts?: readonly number[]; + grpcPorts?: readonly number[]; +} + +const LOOPBACK_HOSTS = new Set([ + 'localhost', + '127.0.0.1', + '::1', + '0.0.0.0', + '::', +]); + +/** Normalize a hostname for comparison (lowercase, strip brackets / zone id). */ +export function normalizeHost(host: string): string { + let h = host.toLowerCase().replace(/\.$/, ''); + if (h.startsWith('[') && h.endsWith(']')) { + h = h.slice(1, -1); + } + const zoneIdx = h.indexOf('%'); + if (zoneIdx !== -1) { + h = h.slice(0, zoneIdx); + } + return h; +} + +/** Collect loopback aliases, hostname, and local interface addresses. */ +export function localHostAddresses(): Set { + const addrs = new Set(LOOPBACK_HOSTS); + addrs.add(normalizeHost(os.hostname())); + for (const ifaces of Object.values(os.networkInterfaces())) { + if (!ifaces) continue; + for (const iface of ifaces) { + addrs.add(normalizeHost(iface.address)); + } + } + return addrs; +} + +/** + * Return true when `urlString` targets a local host and a port this daemon + * is listening on (HTTP/web/MCP or gRPC TCP). + * + * Exported for direct unit tests so the guard cannot silently become a no-op. + */ +export function isSelfConnectionUrl( + urlString: string, + endpoints: { httpPorts: Iterable; grpcPorts: Iterable }, +): boolean { + let url: URL; + try { + url = new URL(urlString); + } catch { + return false; + } + + const defaultPort = url.protocol === 'https:' ? 443 : 80; + const port = url.port ? Number(url.port) : defaultPort; + if (!Number.isInteger(port) || port < 1 || port > 65535) { + return false; + } + + const listenPorts = new Set([ + ...endpoints.httpPorts, + ...endpoints.grpcPorts, + ]); + if (!listenPorts.has(port)) { + return false; + } + + return localHostAddresses().has(normalizeHost(url.hostname)); +} + // ── McpClientPool ────────────────────────────────────────────────────── export class McpClientPool { @@ -42,8 +120,32 @@ export class McpClientPool { /** Max dynamic MCP servers allowed (configurable via config.yaml) */ private maxDynamicServers = 10; + private httpPorts = new Set(); + private grpcPorts = new Set(); + constructor(private registry: ToolRegistry) {} + /** + * Update the daemon listen ports used for self-connection detection. + * Omitted fields are left unchanged. + */ + setListenEndpoints(endpoints: DaemonListenEndpoints): void { + if (endpoints.httpPorts !== undefined) { + this.httpPorts = new Set(endpoints.httpPorts); + } + if (endpoints.grpcPorts !== undefined) { + this.grpcPorts = new Set(endpoints.grpcPorts); + } + } + + /** Current listen endpoints (for tests / diagnostics). */ + getListenEndpoints(): { httpPorts: number[]; grpcPorts: number[] } { + return { + httpPorts: [...this.httpPorts], + grpcPorts: [...this.grpcPorts], + }; + } + /** * Connect to a single config-based MCP server and register its tools. */ @@ -53,10 +155,7 @@ export class McpClientPool { return; } - if (this.isSelfConnection(config)) { - console.warn(`[McpClientPool] Skipping self-connection: ${serverId}`); - return; - } + this.assertNotSelfConnection(serverId, config); // Disconnect existing connection if any if (this.clients.has(serverId)) { @@ -117,6 +216,8 @@ export class McpClientPool { throw new Error(`MCP server '${serverId}' is already registered`); } + this.assertNotSelfConnection(serverId, config); + const dynamicCount = Array.from(this.statuses.values()) .filter(s => s.source === 'dynamic').length; if (dynamicCount >= this.maxDynamicServers) { @@ -415,20 +516,28 @@ export class McpClientPool { } /** - * Guard against connecting to our own MCP server. + * Guard against connecting to our own MCP/HTTP/gRPC endpoints. */ - private isSelfConnection(config: McpServerConfig): boolean { - if (config.transport === 'http' && config.url) { - const url = config.url.toLowerCase(); - // Check for obvious self-referential URLs - if (url.includes('localhost') || url.includes('127.0.0.1')) { - // TODO: compare against our own MCP server port when Phase 4 is implemented - return false; - } + isSelfConnection(config: McpServerConfig): boolean { + if ((config.transport === 'http' || config.transport === 'sse') && config.url) { + return isSelfConnectionUrl(config.url, { + httpPorts: this.httpPorts, + grpcPorts: this.grpcPorts, + }); } return false; } + private assertNotSelfConnection(serverId: string, config: McpServerConfig): void { + if (!this.isSelfConnection(config)) return; + const url = config.url ?? ''; + const msg = + `Refusing self-connection for MCP server '${serverId}': ` + + `URL '${url}' points at this daemon's own listening address/port`; + console.warn(`[McpClientPool] ${msg}`); + throw new Error(msg); + } + /** * Check if config has changed (shallow comparison of key fields). */ diff --git a/packages/daemon/src/daemon/state.ts b/packages/daemon/src/daemon/state.ts index 3b710cf..bbd6601 100644 --- a/packages/daemon/src/daemon/state.ts +++ b/packages/daemon/src/daemon/state.ts @@ -25,8 +25,8 @@ export type { SecretStore } from '../core/secrets.js'; export type { RegisteredTool, ToolPolicyConfig, ToolSourceType } from '../core/tool-registry.js'; export { ToolRegistry } from '../core/tool-registry.js'; export { ToolRouter } from './tool-router.js'; -export { McpClientPool } from './mcp-client-pool.js'; -export type { McpServerStatus } from './mcp-client-pool.js'; +export { McpClientPool, isSelfConnectionUrl, normalizeHost, localHostAddresses } from './mcp-client-pool.js'; +export type { McpServerStatus, DaemonListenEndpoints } from './mcp-client-pool.js'; export { AbbenayMcpServer } from './mcp-server.js'; export { SessionStore } from '../core/session-store.js'; export type { Session, SessionSummary, SessionListOptions, SessionListResult } from '../core/session-store.js'; diff --git a/packages/daemon/src/daemon/web/server.ts b/packages/daemon/src/daemon/web/server.ts index da4c73c..4c23816 100644 --- a/packages/daemon/src/daemon/web/server.ts +++ b/packages/daemon/src/daemon/web/server.ts @@ -1151,6 +1151,9 @@ export async function startEmbeddedWebServer( }); _webPort = port; + + // Keep MCP self-connection guard in sync with the live HTTP listen port + state.mcpClientPool.setListenEndpoints({ httpPorts: [port] }); await new Promise((resolve, reject) => { _httpServer = app.listen(port, () => { diff --git a/packages/daemon/tests/integration/mcp-self-connection.test.ts b/packages/daemon/tests/integration/mcp-self-connection.test.ts new file mode 100644 index 0000000..bde98ae --- /dev/null +++ b/packages/daemon/tests/integration/mcp-self-connection.test.ts @@ -0,0 +1,126 @@ +/** + * E2E: self-connection guard against the daemon's own MCP HTTP endpoint. + * + * Starts an in-process DaemonState + embedded web server with MCP enabled, + * then verifies registration of a self-targeting URL is rejected while a + * non-self mock HTTP MCP URL is allowed (mocked transport connect). + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import * as http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const tmpConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-mcp-self-')); +const tmpSessionsDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-mcp-self-sess-')); + +vi.mock('../../src/core/paths.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getUserConfigPath: () => path.join(tmpConfigDir, 'config.yaml'), + getWorkspaceConfigPath: (wsPath: string) => path.join(wsPath, '.config', 'abbenay', 'config.yaml'), + getSessionsDir: () => tmpSessionsDir, + }; +}); + +vi.mock('../../src/daemon/secrets/keychain.js', () => ({ + KeychainSecretStore: class { + async get(): Promise { return null; } + async set(): Promise {} + async delete(): Promise { return false; } + async has(): Promise { return false; } + }, +})); + +vi.mock('@ai-sdk/mcp', () => ({ + createMCPClient: vi.fn(async () => ({ + tools: vi.fn().mockResolvedValue({}), + listTools: vi.fn().mockResolvedValue({ tools: [{ name: 'ping', description: 'ping' }] }), + close: vi.fn().mockResolvedValue(undefined), + })), +})); + +vi.mock('@ai-sdk/mcp/mcp-stdio', () => ({ + Experimental_StdioMCPTransport: class { + constructor(public opts: unknown) {} + }, +})); + +import { DaemonState } from '../../src/daemon/state.js'; +import { startEmbeddedWebServer, stopEmbeddedWebServer } from '../../src/daemon/web/server.js'; +import { createMCPClient } from '@ai-sdk/mcp'; + +describe('MCP self-connection E2E', () => { + let state: DaemonState; + let webPort: number; + let mockMcp: http.Server; + let mockMcpPort: number; + + beforeAll(async () => { + state = new DaemonState(); + + // Ephemeral web port + const probe = http.createServer(); + await new Promise((resolve) => probe.listen(0, '127.0.0.1', () => resolve())); + webPort = (probe.address() as AddressInfo).port; + await new Promise((resolve, reject) => probe.close((err) => (err ? reject(err) : resolve()))); + + const { port, app } = await startEmbeddedWebServer(state, webPort); + expect(port).toBe(webPort); + await state.mcpServer.start(app); + + // Separate mock "remote" MCP HTTP listener on another port + mockMcp = http.createServer((_req, res) => { + res.writeHead(200); + res.end('ok'); + }); + await new Promise((resolve) => mockMcp.listen(0, '127.0.0.1', () => resolve())); + mockMcpPort = (mockMcp.address() as AddressInfo).port; + }); + + afterAll(async () => { + await state.mcpServer.stop().catch(() => {}); + await state.mcpClientPool.disconnectAll().catch(() => {}); + await stopEmbeddedWebServer(); + await new Promise((resolve) => mockMcp.close(() => resolve())); + fs.rmSync(tmpConfigDir, { recursive: true, force: true }); + fs.rmSync(tmpSessionsDir, { recursive: true, force: true }); + }); + + it('rejects registration when URL points at this daemon /mcp', async () => { + const selfUrls = [ + `http://127.0.0.1:${webPort}/mcp`, + `http://localhost:${webPort}/mcp`, + `http://[::1]:${webPort}/mcp`, + ]; + + for (const [i, url] of selfUrls.entries()) { + await expect( + state.mcpClientPool.connectDynamic(`self-${i}`, { + transport: 'http', + url, + enabled: true, + }), + ).rejects.toThrow(/self-connection/i); + } + + expect(createMCPClient).not.toHaveBeenCalled(); + }); + + it('allows registration of a non-self remote/mock MCP URL', async () => { + vi.mocked(createMCPClient).mockClear(); + + const tools = await state.mcpClientPool.connectDynamic('mock-remote', { + transport: 'http', + url: `http://127.0.0.1:${mockMcpPort}/mcp`, + enabled: true, + }); + + expect(tools.length).toBeGreaterThanOrEqual(1); + expect(state.mcpClientPool.getStatus('mock-remote')?.connected).toBe(true); + expect(createMCPClient).toHaveBeenCalled(); + }); +}); From 4de5aa679de056c7ead0b068673abd84c545d50f Mon Sep 17 00:00:00 2001 From: sathyapramod <8750601+sathyapramod@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:57:06 +0530 Subject: [PATCH 2/3] fix: tolerate partial mcpClientPool mocks after self-connection wiring Optional-chain setListenEndpoints for test doubles, and compare config save paths via realpath on macOS. --- packages/daemon/src/daemon/web/server.ts | 2 +- packages/daemon/tests/integration/http-security.test.ts | 1 + packages/daemon/tests/integration/web-sse.test.ts | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/daemon/src/daemon/web/server.ts b/packages/daemon/src/daemon/web/server.ts index c7f5ac3..1f2f51f 100644 --- a/packages/daemon/src/daemon/web/server.ts +++ b/packages/daemon/src/daemon/web/server.ts @@ -1762,7 +1762,7 @@ export async function startEmbeddedWebServer( _webPort = port; // Keep MCP self-connection guard in sync with the live HTTP listen port - state.mcpClientPool.setListenEndpoints({ httpPorts: [port] }); + state.mcpClientPool?.setListenEndpoints?.({ httpPorts: [port] }); _webHost = bindHost; diff --git a/packages/daemon/tests/integration/http-security.test.ts b/packages/daemon/tests/integration/http-security.test.ts index c8f4572..e5d4a2d 100644 --- a/packages/daemon/tests/integration/http-security.test.ts +++ b/packages/daemon/tests/integration/http-security.test.ts @@ -51,6 +51,7 @@ function createMockState(): DaemonState { mcpClientPool: { getAllStatus() { return []; }, async reconnect() {}, + setListenEndpoints() {}, }, toolRegistry: { listTools() { return []; }, diff --git a/packages/daemon/tests/integration/web-sse.test.ts b/packages/daemon/tests/integration/web-sse.test.ts index 3e243af..ff365a5 100644 --- a/packages/daemon/tests/integration/web-sse.test.ts +++ b/packages/daemon/tests/integration/web-sse.test.ts @@ -404,7 +404,7 @@ describe('Web API - Config Endpoints', () => { expect(statusCode).toBe(200); expect(body.success).toBe(true); const savedPath = path.join(tmpWorkspaceDir, '.config', 'abbenay', 'config.yaml'); - expect(body.path).toBe(savedPath); + expect(body.path).toBe(fs.realpathSync(savedPath)); expect(fs.existsSync(savedPath)).toBe(true); expect(fs.readFileSync(savedPath, 'utf-8')).toContain('openai'); }); From e225ee3b024b1b4f1d8af240e4234a58e901a975 Mon Sep 17 00:00:00 2001 From: sathyapramod <8750601+sathyapramod@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:25:04 +0530 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20H7=20review=20=E2=80=94=20?= =?UTF-8?q?DR-039,=20fail-closed=20local=20hosts,=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renumber self-connection decision to DR-039 (avoids collision with #81), refuse local MCP URLs when listen ports are unknown, normalize IPv4-mapped loopback, record failed status on self-reject, and drop unintentional package-lock engines churn. --- docs/ARCHITECTURE.md | 6 +- docs/decisions.md | 21 ++++--- package-lock.json | 2 +- .../daemon/src/daemon/mcp-client-pool.test.ts | 25 +++++++- packages/daemon/src/daemon/mcp-client-pool.ts | 57 ++++++++++++++++--- 5 files changed, 88 insertions(+), 23 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2e84629..4cab05a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -331,8 +331,10 @@ Manages connections to external MCP servers defined in config. Uses `@ai-sdk/mcp - Hot-reloads when config changes (connects new, disconnects removed) - Refuses self-connections: HTTP/SSE URLs that target this daemon's own listening HTTP or gRPC ports on a local address (`localhost`, `127.0.0.1`, - `::1`, hostname, or any local interface IP) are rejected so the daemon - cannot recurse into its own `/mcp` endpoint + `::1`, IPv4-mapped loopback, hostname, or any local interface IP) are + rejected so the daemon cannot recurse into its own `/mcp` endpoint + (DR-039). When listen ports are not yet registered, local hosts are + refused fail-closed. ```yaml # In config.yaml diff --git a/docs/decisions.md b/docs/decisions.md index f8572c9..69e110d 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -410,8 +410,6 @@ incomplete builds from reaching end users. --- ---- - ## DR-029: Fail-closed TLS for non-loopback gRPC TCP binds **Date:** 2026-07-15 @@ -608,18 +606,23 @@ compare closes token oracle leaks. --- -## DR-038: Block MCP self-connections to the daemon's own endpoints +## DR-039: Block MCP self-connections to the daemon's own endpoints **Date:** 2026-07-20 **Decision:** `McpClientPool` tracks the daemon's HTTP and gRPC listen ports via `setListenEndpoints()` and rejects HTTP/SSE MCP server URLs whose host is -a local address (loopback, hostname, or any local interface IP) and whose port -matches a tracked listen port. Both config `connect()` and dynamic -`connectDynamic()` throw a clear self-connection error. Listen ports are -registered at daemon start (`httpPort` / `grpcPort` options) and when the -embedded web server binds. +a local address (loopback including IPv4-mapped forms such as +`::ffff:127.0.0.1`, hostname, or any local interface IP) and whose port +matches a tracked listen port. When no listen ports are registered yet, local +hosts are refused fail-closed (remote hosts remain allowed). Both config +`connect()` and dynamic `connectDynamic()` throw a clear self-connection error +and record a failed status. Listen ports are registered at daemon start +(`httpPort` / `grpcPort` options) and when the embedded web server binds. +Self URLs may still appear in saved config; rejection is at connect time. **Rationale:** `isSelfConnection()` previously always returned `false`, so the daemon could register its own `/mcp` HTTP endpoint as an external MCP server and amplify tool calls recursively. Comparing target URL host+port against known listen endpoints closes that loop without blocking legitimate remote or -other-localhost MCP servers on different ports. +other-localhost MCP servers on different ports once ports are known. Fail-closed +behavior when the listen set is empty closes the race before +`setListenEndpoints()` runs (finding H7 / AAP-82830). diff --git a/package-lock.json b/package-lock.json index a186433..db65c7e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11200,7 +11200,7 @@ "vitest": "^4.1.9" }, "engines": { - "node": "^20.19.0 || >=22.12.0" + "node": ">=20.0.0" } }, "packages/daemon/node_modules/@grpc/proto-loader": { diff --git a/packages/daemon/src/daemon/mcp-client-pool.test.ts b/packages/daemon/src/daemon/mcp-client-pool.test.ts index 67f1f22..af6e3af 100644 --- a/packages/daemon/src/daemon/mcp-client-pool.test.ts +++ b/packages/daemon/src/daemon/mcp-client-pool.test.ts @@ -404,6 +404,14 @@ describe('isSelfConnectionUrl', () => { expect(isSelfConnectionUrl('HTTP://LocalHost:8787/MCP', endpoints)).toBe(true); }); + it('detects IPv4-mapped IPv6 loopback', () => { + // Node URL canonicalizes ::ffff:127.0.0.1 → ::ffff:7f00:1 + expect(isSelfConnectionUrl('http://[::ffff:127.0.0.1]:8787/mcp', endpoints)).toBe(true); + expect(isSelfConnectionUrl('http://[::ffff:7f00:1]:8787/mcp', endpoints)).toBe(true); + expect(normalizeHost('::ffff:127.0.0.1')).toBe('127.0.0.1'); + expect(normalizeHost('::ffff:7f00:1')).toBe('127.0.0.1'); + }); + it('detects self on gRPC listen port', () => { expect(isSelfConnectionUrl('http://127.0.0.1:50051/', endpoints)).toBe(true); }); @@ -423,8 +431,15 @@ describe('isSelfConnectionUrl', () => { expect(isSelfConnectionUrl('http://localhost:9999/mcp', endpoints)).toBe(false); }); - it('returns false when no listen ports are configured', () => { - expect(isSelfConnectionUrl('http://127.0.0.1:8787/mcp', { + it('fail-closes local hosts when no listen ports are configured', () => { + const empty = { httpPorts: [] as number[], grpcPorts: [] as number[] }; + expect(isSelfConnectionUrl('http://127.0.0.1:8787/mcp', empty)).toBe(true); + expect(isSelfConnectionUrl('http://localhost:3001/sse', empty)).toBe(true); + expect(isSelfConnectionUrl('http://[::1]:9999/mcp', empty)).toBe(true); + }); + + it('allows remote hosts when no listen ports are configured', () => { + expect(isSelfConnectionUrl('http://mcp.example.com:8787/mcp', { httpPorts: [], grpcPorts: [], })).toBe(false); @@ -449,6 +464,9 @@ describe('self-connection rejection', () => { expect(pool.connectedCount).toBe(0); expect(mockCreateMCPClient).not.toHaveBeenCalled(); + const status = pool.getStatus('self'); + expect(status?.connected).toBe(false); + expect(status?.error).toMatch(/self-connection/i); }); it('rejects all localhost URL variants for dynamic registration', async () => { @@ -505,7 +523,8 @@ describe('self-connection rejection', () => { ok: { transport: 'stdio', command: 'ok', enabled: true }, }); - expect(pool.getStatus('self')?.connected).not.toBe(true); + expect(pool.getStatus('self')?.connected).toBe(false); + expect(pool.getStatus('self')?.error).toMatch(/self-connection/i); expect(pool.getStatus('ok')?.connected).toBe(true); }); }); diff --git a/packages/daemon/src/daemon/mcp-client-pool.ts b/packages/daemon/src/daemon/mcp-client-pool.ts index 18ac4cf..e17f87e 100644 --- a/packages/daemon/src/daemon/mcp-client-pool.ts +++ b/packages/daemon/src/daemon/mcp-client-pool.ts @@ -60,6 +60,22 @@ export function normalizeHost(host: string): string { if (zoneIdx !== -1) { h = h.slice(0, zoneIdx); } + // IPv4-mapped IPv6 → plain IPv4 (dotted or Node's ::ffff:7f00:1 form) + if (h.startsWith('::ffff:')) { + const mapped = h.slice('::ffff:'.length); + if (/^\d{1,3}(?:\.\d{1,3}){3}$/.test(mapped)) { + h = mapped; + } else { + const parts = mapped.split(':'); + if (parts.length === 2) { + const hi = Number.parseInt(parts[0], 16); + const lo = Number.parseInt(parts[1], 16); + if (Number.isFinite(hi) && Number.isFinite(lo)) { + h = `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`; + } + } + } + } return h; } @@ -77,8 +93,11 @@ export function localHostAddresses(): Set { } /** - * Return true when `urlString` targets a local host and a port this daemon - * is listening on (HTTP/web/MCP or gRPC TCP). + * Return true when `urlString` would recurse into this daemon. + * + * - When listen ports are known: local host + matching HTTP/gRPC port → self. + * - When listen ports are empty: any local host → self (fail-closed). + * - Remote hosts are never treated as self. * * Exported for direct unit tests so the guard cannot silently become a no-op. */ @@ -99,15 +118,23 @@ export function isSelfConnectionUrl( return false; } + const host = normalizeHost(url.hostname); + const isLocal = localHostAddresses().has(host); + if (!isLocal) { + return false; + } + const listenPorts = new Set([ ...endpoints.httpPorts, ...endpoints.grpcPorts, ]); - if (!listenPorts.has(port)) { - return false; + + // Fail-closed: unknown listen set → refuse all local hosts + if (listenPorts.size === 0) { + return true; } - return localHostAddresses().has(normalizeHost(url.hostname)); + return listenPorts.has(port); } // ── McpClientPool ────────────────────────────────────────────────────── @@ -155,7 +182,7 @@ export class McpClientPool { return; } - this.assertNotSelfConnection(serverId, config); + this.assertNotSelfConnection(serverId, config, 'config'); // Disconnect existing connection if any if (this.clients.has(serverId)) { @@ -216,7 +243,7 @@ export class McpClientPool { throw new Error(`MCP server '${serverId}' is already registered`); } - this.assertNotSelfConnection(serverId, config); + this.assertNotSelfConnection(serverId, config, 'dynamic', scope); const dynamicCount = Array.from(this.statuses.values()) .filter(s => s.source === 'dynamic').length; @@ -528,13 +555,27 @@ export class McpClientPool { return false; } - private assertNotSelfConnection(serverId: string, config: McpServerConfig): void { + private assertNotSelfConnection( + serverId: string, + config: McpServerConfig, + source: McpServerSource, + scope?: DynamicScope, + ): void { if (!this.isSelfConnection(config)) return; const url = config.url ?? ''; const msg = `Refusing self-connection for MCP server '${serverId}': ` + `URL '${url}' points at this daemon's own listening address/port`; console.warn(`[McpClientPool] ${msg}`); + this.statuses.set(serverId, { + id: serverId, + config: { ...config, enabled: true }, + connected: false, + toolCount: 0, + error: msg, + source, + scope, + }); throw new Error(msg); }