diff --git a/clients/cli/__tests__/cli.test.ts b/clients/cli/__tests__/cli.test.ts index d50cd861..bfb22c03 100644 --- a/clients/cli/__tests__/cli.test.ts +++ b/clients/cli/__tests__/cli.test.ts @@ -24,6 +24,23 @@ import { createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; +import { createServer, request } from "node:http"; + +/** + * Every proxy variable undici's EnvHttpProxyAgent consults. The proxy test + * clears all of them rather than only setting HTTP_PROXY: the agent prefers the + * lowercase forms and honors NO_PROXY dynamically, so an ambient value in the + * developer's or CI's environment could route the request elsewhere, or bypass + * the test's proxy entirely, and fail the assertion for the wrong reason. + */ +const PROXY_ENV_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", +] as const; describe("CLI Tests", () => { describe("Basic CLI Mode", () => { @@ -326,6 +343,81 @@ describe("CLI Tests", () => { }); }); + describe("HTTP proxy (#2067)", () => { + it("routes the MCP connection through HTTP_PROXY (#2067)", async () => { + // Proxy support is one line in cli.ts — `fetch: createProxyFetch()` on the + // environment — and it is what puts the proxy at the BOTTOM of the fetch + // stack, where InspectorClient's own wrappers compose over it rather than + // discarding it. `InspectorClient` sets `fetchFn` unconditionally, so the + // transport-level fallback never fires for this client; delete that line + // and CLI proxy support disappears with every core test still green. + // + // Asserted behaviorally, through a real forwarding proxy, rather than by + // inspecting the environment object: the bug this replaces was a runtime + // dispatch failure that no structural assertion would have caught. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + const seen: string[] = []; + const proxy = createServer((req, res) => { + seen.push(req.url ?? ""); + const target = new URL(req.url ?? ""); + const upstream = request( + { + host: target.hostname, + port: target.port, + path: target.pathname + target.search, + method: req.method, + headers: req.headers, + }, + (up) => { + res.writeHead(up.statusCode ?? 502, up.headers); + up.pipe(res); + }, + ); + upstream.on("error", () => { + res.writeHead(502); + res.end(); + }); + req.pipe(upstream); + }); + const savedProxyEnv: Record = {}; + for (const name of PROXY_ENV_VARS) { + savedProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + try { + await server.start(); + await new Promise((r) => proxy.listen(0, "127.0.0.1", () => r())); + const addr = proxy.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + process.env.HTTP_PROXY = `http://127.0.0.1:${port}`; + + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + ]); + + expectCliSuccess(result); + expect(JSON.stringify(expectValidJson(result))).toContain("echo"); + // The point of the test: it did not go direct. + expect(seen.length).toBeGreaterThan(0); + expect(seen.every((u) => u.startsWith(server.url))).toBe(true); + } finally { + for (const name of PROXY_ENV_VARS) { + const previous = savedProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } + await new Promise((r) => proxy.close(() => r())); + await server.stop(); + } + }); + }); + describe("Roots capability (#1797)", () => { it("answers a server's roots/list instead of -32601", async () => { // The CLI used to omit `roots` when constructing its InspectorClient, so diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 56ac7682..74bb2d7a 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -102,7 +102,14 @@ const h = vi.hoisted(() => { // handler for the event (the common single-server case). type EventEntry = { client: unknown; fn: (event: unknown) => void }; const clientEvents = new Map>(); - const clientInstances: Array<{ cfg?: { type?: string; url?: string } }> = []; + const clientInstances: Array<{ + cfg?: { type?: string; url?: string }; + opts?: { environment?: { fetch?: unknown } }; + }> = []; + // What the mocked `createProxyFetch` returns. `undefined` is the real + // behavior with no proxy env var set; a test points this at a sentinel to + // exercise the proxy-configured branch (#2067). + const proxyFetchState: { current: unknown } = { current: undefined }; const fireClientEvent = (event: string, detail?: unknown) => { clientEvents.get(event)?.forEach((e) => e.fn({ detail })); }; @@ -127,8 +134,15 @@ const h = vi.hoisted(() => { } class FakeClient { cfg: { type?: string; url?: string } | undefined; - constructor(config?: { type?: string; url?: string }) { + // Kept so a test can assert what App.tsx put in the environment — the proxy + // fetch wiring (#2067) is otherwise invisible to these mock-driven tests. + opts: { environment?: { fetch?: unknown } } | undefined; + constructor( + config?: { type?: string; url?: string }, + opts?: { environment?: { fetch?: unknown } }, + ) { this.cfg = config; + this.opts = opts; clientInstances.push(this); } // Derive the transport type from the server config the client was built @@ -183,6 +197,7 @@ const h = vi.hoisted(() => { } return { ctrl, + proxyFetchState, connect, disconnect, openUrl, @@ -232,10 +247,12 @@ vi.mock("@inspector/core/mcp/state/index.js", () => ({ })); vi.mock("@inspector/core/mcp/node/index.js", () => ({ createTransportNode: vi.fn(), - // Returns undefined for "no proxy configured", which is what the real one - // does with no proxy env var set — so `environment.fetch` stays unset and the - // client falls back to the built-in fetch, exactly as before #2067. - createProxyFetch: vi.fn(() => undefined), + // Defaults to undefined — "no proxy configured", which is what the real one + // returns with no proxy env var set, so `environment.fetch` stays unset and + // the client falls back to the built-in fetch, exactly as before #2067. A + // test can point `h.proxyFetchResult` at a sentinel to exercise the other + // branch. + createProxyFetch: vi.fn(() => h.proxyFetchState.current), })); vi.mock("@inspector/core/react/useInspectorClient.js", () => ({ useInspectorClient: h.useInspectorClient, @@ -637,6 +654,7 @@ beforeEach(() => { h.callbackStop.mockClear(); h.clientEvents.clear(); h.clientInstances.length = 0; + h.proxyFetchState.current = undefined; h.runner.override = null; h.clientSpies.authenticate.mockReset(); h.clientSpies.authenticate.mockResolvedValue( @@ -1612,6 +1630,27 @@ describe("App (OAuth result branches)", () => { await expectFrame(r, "Authorization updated. Retry your action"); }); + it("installs the proxy fetch as environment.fetch when a proxy is configured", async () => { + // The bottom-of-stack wiring for #2067. It is one line in App.tsx and it is + // what makes InspectorClient's own wrappers compose OVER the proxy instead + // of discarding it — delete it and TUI proxy support silently disappears + // while every other test stays green. + const sentinel = (async () => new Response("")) as unknown as typeof fetch; + h.proxyFetchState.current = sentinel; + await mount(oneHttp()); + expect(h.clientInstances.length).toBeGreaterThan(0); + expect(h.clientInstances[0]!.opts?.environment?.fetch).toBe(sentinel); + }); + + it("leaves environment.fetch unset when no proxy is configured", async () => { + // The default path must stay on the built-in fetch — a wrapper installed + // unconditionally would put every TUI user behind an undici fetch. + h.proxyFetchState.current = undefined; + await mount(oneHttp()); + expect(h.clientInstances.length).toBeGreaterThan(0); + expect(h.clientInstances[0]!.opts?.environment?.fetch).toBeUndefined(); + }); + it("ignores auth lifecycle events from a non-selected server", async () => { const r = await mount(twoHttp()); // web is selected; api is not await press(r, ["a"]); diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 79475bc4..7716b230 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -919,7 +919,12 @@ export interface InspectorClientEnvironment { /** * Optional fetch function for HTTP requests (OAuth discovery/token exchange and * MCP transport). When provided, used for both auth and transport to bypass CORS. - * - Node: undefined (uses global fetch) + * - Node: `createProxyFetch()` (core/mcp/node/proxyFetch.ts) — a proxy-aware + * fetch when `HTTPS_PROXY`/`HTTP_PROXY` is set, and `undefined` otherwise, + * which leaves the built-in global fetch in place. This is the BOTTOM of the + * fetch stack: `InspectorClient` wraps whatever it finds here, so proxying + * composes with request tracking and the OAuth endpoint overrides instead of + * being discarded by them (#2067). * - Browser: createRemoteFetch */ fetch?: typeof fetch;