From 745fe5d40b9aa5f66788cc62de9cf15fef623a5a Mon Sep 17 00:00:00 2001 From: "DESKTOP-5P6LRD2\\MLL" Date: Wed, 16 Sep 2026 00:49:21 -0300 Subject: [PATCH] fix(mcp): harden HTTP transport and improve disconnect diagnostics Refs #297. Related to #140. Intermittent MCP connection failures can occur while the DevSpace process remains running and both the local endpoint and public tunnel remain reachable. This draft addresses server-side transport and authentication weaknesses encountered during that investigation; it does not claim to resolve every disconnect in #297. The listener uses a five-minute keep-alive timeout and a 305-second headers timeout to reduce premature origin socket closure between requests through a reverse proxy. Bearer authentication runs directly as Express middleware, removing a Promise wrapper that could remain unresolved when authentication was rejected. Finite MCP responses use JSON mode and are buffered before sending headers, allowing an explicit Content-Length and preventing body-read failures from producing an already-started successful response. subscriptions/listen retains SSE. The adapter adds a 20-second timeout while awaiting handler.fetch and propagates client disconnection through an abort signal. Loopback-bound servers automatically trust loopback proxies. Logging records request start, completion, premature closure, and MCP protocol/method metadata to help distinguish requests reaching DevSpace from failures earlier in the path. Validation was rerun on this dedicated branch on Windows: pnpm typecheck, pnpm build, and git diff --check passed. The server, server-oauth, and server-shutdown test files passed all 29 tests with no failures or skips. Coverage includes modern/legacy HTTP MCP, OAuth resource enforcement, proxy configuration, listener timeouts, Content-Length, and shutdown behavior. These checks do not establish sustained reliability through the real ChatGPT-to-Funnel path. This is a draft because JSON mode drops intermediate notifications for ordinary calls, the buffering/timeout policy needs maintainer review, and intermittent connector failures were still observed during the earlier investigation. Some failures had no corresponding incoming server request. The separate yield-parameter mismatch in #297 is not addressed, and the Windows process-cleanup fallback from our fork is excluded. --- docs/configuration.md | 15 ++++ src/cli.ts | 3 +- src/server.test.ts | 52 +++++++++++- src/server.ts | 180 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 226 insertions(+), 24 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 408018c7f..4811de7ee 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -79,6 +79,21 @@ Omitted sections and keys use the defaults shown above. An empty `workspaces.allowedRoots` uses the current working directory. Unknown keys are rejected so spelling mistakes cannot silently alter behavior. +### Reverse proxies and tunnels + +When `server.host` is `127.0.0.1`, `localhost`, or `::1`, DevSpace automatically +trusts only loopback reverse proxies. This is the normal topology for Tailscale +Serve/Funnel, Cloudflare Tunnel, and similar local tunnel agents, and allows +their forwarded client IP headers to work with OAuth rate limiting without +trusting arbitrary remote proxies. + +Leave `server.trustProxy` set to `false` for that topology. Set it to `true` +only when DevSpace is intentionally behind a trusted non-loopback proxy; that +explicit setting tells Express to trust forwarded proxy information generally. + +DevSpace keeps HTTP connections open for five minutes so a tunnel can safely +reuse them between MCP calls instead of racing Node's short default timeout. + `oauth.allowedResourceUrls` accepts exact alternate MCP resource URLs for clients that connect through a resource alias, such as a secure MCP tunnel. The normal `server.publicBaseUrl` `/mcp` resource remains allowed automatically. diff --git a/src/cli.ts b/src/cli.ts index 3037f65e8..f36f4127c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -324,7 +324,7 @@ async function serve(): Promise { const config = loadConfig(); await runStartupWorktreeCleanup(config); - const { createServer } = await import("./server.js"); + const { configureHttpServer, createServer } = await import("./server.js"); const { app, close, localAgentProviders } = createServer(config); const httpServer = app.listen(config.port, config.host, () => { console.log(`devspace listening on http://${config.host}:${config.port}/mcp`); @@ -338,6 +338,7 @@ async function serve(): Promise { console.log(`logging: ${config.logging.level} ${config.logging.format}`); console.log(`subagent providers: ${formatLocalAgentProviderStatusSummary(localAgentProviders)}`); }); + configureHttpServer(httpServer); let shuttingDown = false; const shutdown = async () => { diff --git a/src/server.test.ts b/src/server.test.ts index 59bcc5d37..c2c2ccc5f 100644 --- a/src/server.test.ts +++ b/src/server.test.ts @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import { access, mkdtemp, mkdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; +import { createServer as createHttpServer } from "node:http"; import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import test, { type TestContext } from "node:test"; @@ -14,7 +15,13 @@ import { buildLocalAgentProviderStatuses } from "./local-agent-catalog.js"; import type { SubagentsConfig } from "./local-agent-config.js"; import { createReviewCheckpointManager } from "./review-checkpoints.js"; import { ProcessSessionManager } from "./process-sessions.js"; -import { createMcpServer, createServer } from "./server.js"; +import { + configureHttpServer, + createMcpServer, + createServer, + DEVSPACE_HTTP_HEADERS_TIMEOUT_MS, + DEVSPACE_HTTP_KEEP_ALIVE_TIMEOUT_MS, +} from "./server.js"; import { SqliteWorkspaceStore } from "./workspace-store.js"; import { WorkspaceRegistry } from "./workspaces.js"; import { writeTestDevspaceConfig } from "./test-support/config.test.js"; @@ -470,6 +477,43 @@ test("open_workspace scopes checkout reuse to OpenAI session metadata", async (t assert.ok(Array.isArray(structuredContent(unscoped).agents_files)); }); +test("server trusts only loopback reverse proxies automatically", async (t) => { + const root = await mkdtemp(join(tmpdir(), "devspace-trust-proxy-test-")); + t.after(async () => rm(root, { recursive: true, force: true })); + + const cases = [ + { host: "127.0.0.1", trustProxy: false, expected: "loopback" }, + { host: "localhost", trustProxy: false, expected: "loopback" }, + { host: "0.0.0.0", trustProxy: false, expected: false }, + { host: "0.0.0.0", trustProxy: true, expected: true }, + ] as const; + + for (const [index, testCase] of cases.entries()) { + const config = loadConfig(writeTestDevspaceConfig(join(root, `config-${index}`), { + server: { + host: testCase.host, + port: 1, + publicBaseUrl: "https://example.test", + trustProxy: testCase.trustProxy, + }, + storage: { stateDir: join(root, `state-${index}`) }, + workspaces: { allowedRoots: [root] }, + logging: { level: "silent" }, + })); + const running = createServer(config, { incomingArtifactAdapters: [] }); + assert.equal(running.app.get("trust proxy"), testCase.expected); + await running.close(); + } +}); + +test("HTTP listener uses a tunnel-safe keep-alive timeout", () => { + const httpServer = createHttpServer(); + configureHttpServer(httpServer); + + assert.equal(httpServer.keepAliveTimeout, DEVSPACE_HTTP_KEEP_ALIVE_TIMEOUT_MS); + assert.equal(httpServer.headersTimeout, DEVSPACE_HTTP_HEADERS_TIMEOUT_MS); +}); + test("HTTP endpoint serves modern MCP and stateless legacy clients", async (t) => { const { root, localBaseUrl, accessToken } = await httpServerFixture( t, @@ -503,6 +547,8 @@ test("HTTP endpoint serves modern MCP and stateless legacy clients", async (t) = {}, ); assert.equal(listed.status, 200, await listed.clone().text()); + assert.ok(Number(listed.headers.get("content-length")) > 0); + assert.equal(listed.headers.get("transfer-encoding"), null); const listBody = await listed.json() as { result?: { tools?: Array<{ name?: string }> }; }; @@ -671,6 +717,7 @@ interface HttpServerFixture { root: string; localBaseUrl: string; accessToken: string; + httpServer: ReturnType; running: ReturnType; } @@ -694,6 +741,7 @@ async function httpServerFixture( const running = createServer(config, { incomingArtifactAdapters: [] }); const httpServer = running.app.listen(0, "127.0.0.1"); await new Promise((resolve) => httpServer.once("listening", resolve)); + configureHttpServer(httpServer); t.after(async () => { await new Promise((resolve, reject) => { @@ -711,7 +759,7 @@ async function httpServerFixture( config.publicBaseUrl, ownerToken, ); - return { root, localBaseUrl, accessToken, running }; + return { root, localBaseUrl, accessToken, httpServer, running }; } async function fixture( diff --git a/src/server.ts b/src/server.ts index 0a2792f3f..a2571d789 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { readFileSync } from "node:fs"; import { access, realpath } from "node:fs/promises"; +import type { Server as HttpServer } from "node:http"; import { fileURLToPath } from "node:url"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { createMcpExpressApp } from "@modelcontextprotocol/sdk/server/express.js"; @@ -8,7 +9,7 @@ import { mcpAuthRouter, getOAuthProtectedResourceMetadataUrl } from "@modelconte import { requireBearerAuth } from "@modelcontextprotocol/sdk/server/auth/middleware/bearerAuth.js"; import { resourceUrlFromServerUrl } from "@modelcontextprotocol/sdk/shared/auth-utils.js"; import { createMcpHandler } from "@modelcontextprotocol/server"; -import { toNodeHandler } from "@modelcontextprotocol/node"; +import { toWebRequest } from "@modelcontextprotocol/node"; import { registerAppResource, registerAppTool, @@ -92,6 +93,125 @@ interface RunningServer { close(): Promise; } +// Keep the origin socket open longer than the short default so a reverse +// proxy or tunnel cannot reuse a socket after Node has already closed it. +export const DEVSPACE_HTTP_KEEP_ALIVE_TIMEOUT_MS = 5 * 60 * 1_000; +export const DEVSPACE_HTTP_HEADERS_TIMEOUT_MS = + DEVSPACE_HTTP_KEEP_ALIVE_TIMEOUT_MS + 5_000; + +export function configureHttpServer(httpServer: HttpServer): void { + httpServer.keepAliveTimeout = DEVSPACE_HTTP_KEEP_ALIVE_TIMEOUT_MS; + httpServer.headersTimeout = DEVSPACE_HTTP_HEADERS_TIMEOUT_MS; +} + +type ExpressTrustProxySetting = false | true | "loopback"; + +function expressTrustProxySetting(config: ServerConfig): ExpressTrustProxySetting { + if (config.logging.trustProxy) return true; + if (["localhost", "127.0.0.1", "::1"].includes(config.host)) return "loopback"; + return false; +} + +function formatTrustProxySetting(setting: ExpressTrustProxySetting): string { + if (setting === "loopback") return "loopback (automatic)"; + return setting ? "enabled (configured)" : "disabled"; +} + +function isLongLivedMcpRequest(body: unknown): boolean { + if (!body || typeof body !== "object" || Array.isArray(body)) return false; + return (body as { method?: unknown }).method === "subscriptions/listen"; +} + +function rpcRequestLogFields(body: unknown): Record { + if (!body || typeof body !== "object" || Array.isArray(body)) return {}; + const candidate = body as { id?: unknown; method?: unknown }; + return { + rpcId: candidate.id, + rpcIdType: typeof candidate.id, + rpcMethod: candidate.method, + }; +} + +const DEVSPACE_MCP_HANDLER_TIMEOUT_MS = 20_000; + +function createReliableMcpNodeHandler( + handler: ReturnType, + onerror: (error: Error) => void, +) { + return async (req: Request, res: Response, parsedBody: unknown): Promise => { + let finished = false; + const abort = new AbortController(); + const handleClose = () => { + if (!finished) abort.abort(); + }; + res.on("close", handleClose); + + try { + const webRequest = await toWebRequest(req, parsedBody, { signal: abort.signal }); + const response = await Promise.race([ + handler.fetch(webRequest, { + ...(req.auth !== undefined ? { authInfo: req.auth } : {}), + ...(parsedBody !== undefined ? { parsedBody } : {}), + }), + new Promise((_, reject) => { + const timer = setTimeout(() => { + abort.abort(); + reject(new Error(`MCP handler timed out after ${DEVSPACE_MCP_HANDLER_TIMEOUT_MS}ms`)); + }, DEVSPACE_MCP_HANDLER_TIMEOUT_MS); + timer.unref?.(); + abort.signal.addEventListener("abort", () => clearTimeout(timer), { once: true }); + }), + ]); + const headers: Record = {}; + for (const [name, value] of response.headers) headers[name] = value; + + if (response.body === null) { + res.writeHead(response.status, headers); + finished = true; + res.end(); + return; + } + + if (isLongLivedMcpRequest(parsedBody)) { + res.writeHead(response.status, headers); + for await (const chunk of response.body) { + if (abort.signal.aborted) break; + if (!res.write(chunk)) { + await new Promise((resolve) => res.once("drain", resolve)); + } + } + finished = true; + res.end(); + return; + } + + // Finite MCP responses are buffered before writing them to Node. This + // gives the client an explicit Content-Length and prevents a stream + // error from turning into a misleading HTTP 200 with a truncated body. + const body = Buffer.from(await response.arrayBuffer()); + delete headers["transfer-encoding"]; + headers["content-length"] = String(body.byteLength); + res.writeHead(response.status, headers); + finished = true; + res.end(body); + } catch (error) { + const normalized = error instanceof Error ? error : new Error(String(error)); + try { + onerror(normalized); + } catch { + // Logging must not mask the transport error. + } + if (!res.headersSent) { + sendJsonRpcError(res, 500, -32603, "Internal server error"); + } else if (!res.destroyed) { + res.destroy(normalized); + } + } finally { + res.off("close", handleClose); + } + }; +} + type TrackToolActivity = (operation: () => Promise) => Promise; class ToolActivityTracker { @@ -209,7 +329,7 @@ function sendJsonRpcError( function requestLogFields(req: Request, config: ServerConfig): Record { return { - ip: requestIp(req, config.logging.trustProxy), + ip: requestIp(req, expressTrustProxySetting(config) !== false), host: req.header("host"), userAgent: req.header("user-agent"), origin: req.header("origin"), @@ -855,25 +975,34 @@ export function createServer( return adapter.server; }, { legacy: "stateless", + responseMode: "json", onerror: logMcpHandlerError, }); - const mcpNodeHandler = toNodeHandler(mcpHandler, { - onerror: logMcpHandlerError, - }); + const mcpNodeHandler = createReliableMcpNodeHandler(mcpHandler, logMcpHandlerError); - if (config.logging.trustProxy) { - app.set("trust proxy", true); - } + app.set("trust proxy", expressTrustProxySetting(config)); app.use((req, res, next) => { const requestId = randomUUID(); const startedAt = performance.now(); + const path = requestPath(req); + const shouldLogRequest = config.logging.requests + && (config.logging.assets || !path.startsWith("/mcp-app-assets")); + let finished = false; res.locals.requestId = requestId; + if (shouldLogRequest) { + logEvent(config.logging, "debug", "http_request_start", { + requestId, + method: req.method, + path, + ...requestLogFields(req, config), + }); + } + res.on("finish", () => { - const path = requestPath(req); - if (!config.logging.requests) return; - if (!config.logging.assets && path.startsWith("/mcp-app-assets")) return; + finished = true; + if (!shouldLogRequest) return; logEvent(config.logging, "info", "http_request", { requestId, @@ -885,6 +1014,18 @@ export function createServer( }); }); + res.on("close", () => { + if (finished || !shouldLogRequest) return; + logEvent(config.logging, "warn", "http_request_aborted", { + requestId, + method: req.method, + path, + status: res.statusCode, + durationMs: Math.round(performance.now() - startedAt), + ...requestLogFields(req, config), + }); + }); + next(); }); @@ -918,17 +1059,9 @@ export function createServer( res.json({ ok: true, name: "devspace" }); }); - app.all("/mcp", async (req, res) => { + app.all("/mcp", bearerAuth, async (req, res) => { const requestId = res.locals.requestId as string | undefined; - await new Promise((resolve, reject) => { - bearerAuth(req, res, (error?: unknown) => { - if (error) reject(error); - else resolve(); - }); - }); - if (res.headersSent) return; - if (!req.auth?.resource || !oauthProvider.isResourceAllowed(req.auth.resource)) { logEvent(config.logging, "warn", "auth_denied", { requestId, @@ -944,6 +1077,10 @@ export function createServer( logEvent(config.logging, "debug", "mcp_request", { requestId, method: req.method, + protocolVersion: req.header("mcp-protocol-version"), + mcpMethod: req.header("mcp-method"), + mcpName: req.header("mcp-name"), + ...rpcRequestLogFields(req.body), }); try { @@ -1002,7 +1139,7 @@ if (await isMainModule()) { console.log(`logging: ${config.logging.level} ${config.logging.format}`); console.log(`request logging: ${config.logging.requests ? "enabled" : "disabled"}`); console.log(`asset logging: ${config.logging.assets ? "enabled" : "disabled"}`); - console.log(`trust proxy: ${config.logging.trustProxy ? "enabled" : "disabled"}`); + console.log(`trust proxy: ${formatTrustProxySetting(expressTrustProxySetting(config))}`); const artifactDownloadStatus = !config.artifactsEnabled ? "disabled" : isArtifactDownloadSupportedPlatform() @@ -1011,6 +1148,7 @@ if (await isMainModule()) { console.log(`native artifact download: ${artifactDownloadStatus}`); console.log(`subagent providers: ${formatLocalAgentProviderStatusSummary(localAgentProviders)}`); }); + configureHttpServer(httpServer); let shuttingDown = false; const shutdown = async () => {