From 96f96aad41a0924ecb4e82f334b465f06fe895e6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 22:18:43 -0400 Subject: [PATCH] fix(proxy): preserve fetch semantics through the proxy adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review round 4 on #2099 — three posted comments and three suppressed ones, all valid. - A rebuilt Response reported `url: ""`, `redirected: false`, and `type: "default"` even after undici followed a redirect: a fetch-contract regression visible only to proxy users, and one `core/auth/endpointOverrides.ts` explicitly guards against by declining to rebuild at all. This adapter has no such choice — the response arrives from the wrong realm — so it now reinstates the three with `defineProperty`. - Normalizing a `Request` input dropped every option the Request itself carried beyond method/headers/body/signal. Most visibly `redirect`: a `new Request(url, { redirect: "manual" })` started following redirects instead of surfacing them. All of them are now carried through, with an explicit `init` still winning per the fetch contract. Both are covered by tests driving a real redirect through a real proxy, and both were confirmed to fail without the fix. - `@modelcontextprotocol/ext-apps` was added only to the CLI's `external` list. AGENTS.md requires every root-declared runtime dependency in all three, since which client reaches it is a function of what `core/` imports rather than of the client's own code. Added to the web runner and TUI lists too. - The `BUNDLED_CLIENTS` test compared the hard-coded list against a hard-coded copy of itself, so a new tsup-bundled client would have left it green while going unchecked. It now discovers clients from disk (a client is tsup-bundled iff it ships a tsup config) and compares. Verified it fails when a stub client is added. - AGENTS.md's project tree still described `core/mcp/node/` as only the stdio transport factory; it now carries `proxyFetch.ts` and its role. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- AGENTS.md | 6 ++- clients/cli/tsup.config.ts | 8 +-- clients/tui/tsup.config.ts | 6 +++ .../integration/mcp/node/transport.test.ts | 36 ++++++++++++++ clients/web/tsup.runner.config.ts | 6 +++ core/mcp/node/proxyFetch.ts | 41 ++++++++++++++-- scripts/verify-bundle-externals.test.mjs | 49 ++++++++++++++++--- 7 files changed, 135 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 48831c45..e7acd609 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -169,7 +169,11 @@ v2/main/ │ │ │ # server.json parser, strategy registry + well-known │ │ │ # paths, strategy-agnostic merge. Pure/isomorphic; │ │ │ # used by the web file-upload path + /api/import-source. -│ │ ├── node/ # Node stdio transport factory +│ │ ├── node/ # Node stdio transport factory, plus proxyFetch.ts +│ │ │ # (the shared HTTPS_PROXY/HTTP_PROXY/NO_PROXY +│ │ │ # fetch every Node client installs as its +│ │ │ # `environment.fetch`, and the web backend as the +│ │ │ # createTransportNode default — #2067) │ │ ├── remote/ # Browser HTTP/SSE transport + remote logger/fetch │ │ │ └── node/ # Hono-based remote server backend (used by remote/ above) │ │ └── state/ # InspectorClient state stores consumed by core/react/ diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index e0463a3b..74e70568 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -39,10 +39,10 @@ export default defineConfig({ "@modelcontextprotocol/client", "@modelcontextprotocol/core", // Root-declared and reached through `core/mcp/apps.ts`, so it must be - // external here like every other root runtime dependency — it was being - // inlined (dragging the transitive v1 SDK and zod-to-json-schema with it), - // which the #2067 guard surfaced. ESM, so it was not failing the way - // `undici` did; the rule is what it violated. + // external here like every other root runtime dependency (AGENTS.md). This + // client was actually inlining it — dragging the transitive v1 SDK and + // zod-to-json-schema in with it — which the #2067 guard surfaced. ESM, so it + // was not failing the way `undici` did; the rule is what it violated. "@modelcontextprotocol/ext-apps", "commander", "pino", diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 059ecc6a..60fc9827 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -127,6 +127,12 @@ export default defineConfig({ "pino", "@modelcontextprotocol/client", "@modelcontextprotocol/core", + // Root-declared and reached through `core/mcp/apps.ts`, so it must be + // external here like every other root runtime dependency — which client + // actually reaches it is a function of what `core/` imports, not of this + // client's own code, so all three lists carry it (AGENTS.md). The CLI was + // inlining it; the #2067 guard surfaced that. + "@modelcontextprotocol/ext-apps", "@napi-rs/keyring", // Root-declared (see the repo's dependency-placement rule) and CJS, which // is the combination that bites: tsup externalizes what the *client's* diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index d8615cab..c30cec3c 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -467,6 +467,11 @@ describe("Transport", () => { res.end(); return; } + if (req.url === "/redirect") { + res.writeHead(302, { location: `${originUrl}/landed` }); + res.end(); + return; + } res.writeHead(200, { "content-type": "text/plain", "x-origin": "1" }); res.end("hello from origin"); }); @@ -552,6 +557,37 @@ describe("Transport", () => { expect(res.body).toBeNull(); }); + it("reports url/redirected/type after following a redirect", async () => { + // A rebuilt Response cannot carry these through its constructor, so they + // are reinstated explicitly. Without that, a proxied response reports + // `url: ""` and `redirected: false` even when undici did follow a + // redirect — a fetch-contract regression only proxy users would see. + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + + const res = await proxyFetch!(`${originUrl}/redirect`); + expect(res.status).toBe(200); + expect(res.url).toBe(`${originUrl}/landed`); + expect(res.redirected).toBe(true); + expect(res.type).toBe("basic"); + }); + + it("honors a Request's own redirect mode", async () => { + // The Request carries options the adapter must not drop on the way into + // undici. `redirect: "manual"` is the visible one: lose it and a caller + // asking to see the 302 silently gets the followed 200 instead. + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + + const res = await proxyFetch!( + new Request(`${originUrl}/redirect`, { redirect: "manual" }), + ); + expect(res.status).toBe(302); + expect(res.redirected).toBe(false); + }); + it("accepts a URL instance and forwards the init", async () => { clearProxyEnv(); process.env.HTTP_PROXY = proxyUrl; diff --git a/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index ae36e831..2fb61e0d 100644 --- a/clients/web/tsup.runner.config.ts +++ b/clients/web/tsup.runner.config.ts @@ -44,6 +44,12 @@ export default defineConfig({ "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", + // Root-declared and reached through `core/mcp/apps.ts`, so it must be + // external here like every other root runtime dependency — which client + // actually reaches it is a function of what `core/` imports, not of this + // client's own code, so all three lists carry it (AGENTS.md). The CLI was + // inlining it; the #2067 guard surfaced that. + "@modelcontextprotocol/ext-apps", ], esbuildOptions(options) { options.alias = { diff --git a/core/mcp/node/proxyFetch.ts b/core/mcp/node/proxyFetch.ts index b19265be..14e8a068 100644 --- a/core/mcp/node/proxyFetch.ts +++ b/core/mcp/node/proxyFetch.ts @@ -67,14 +67,22 @@ const NULL_BODY_STATUSES: ReadonlySet = new Set([ * * The body passes through as a `ReadableStream` rather than being buffered, so * SSE responses (the streamable-HTTP transport's whole event channel) still - * stream. `url` and `redirected` are not carried over — the `Response` - * constructor cannot set them — which is safe here only because nothing in this - * repo or the SDK reads either. + * stream. + * + * `url`, `redirected`, and `type` are restored afterwards with + * `defineProperty`, because the `Response` constructor cannot set them and a + * rebuilt response would otherwise report `url: ""` and `redirected: false` + * even after undici followed a redirect — a fetch-contract regression visible + * only to proxy users. `core/auth/endpointOverrides.ts` guards the same three + * properties by declining to rebuild at all; this adapter has no such choice, + * since the response arrives from the wrong realm, so it reinstates them + * instead. They shadow the prototype getters as own properties, which is what + * `Response` exposes them as to a consumer. */ function toGlobalResponse(res: UndiciResponse): Response { const headers = new Headers(); for (const [key, value] of res.headers) headers.append(key, value); - return new Response( + const out = new Response( NULL_BODY_STATUSES.has(res.status) ? null : asBodyInit(res.body), { status: res.status, @@ -82,6 +90,18 @@ function toGlobalResponse(res: UndiciResponse): Response { headers, }, ); + for (const [key, value] of [ + ["url", res.url], + ["redirected", res.redirected], + ["type", res.type], + ] as const) { + Object.defineProperty(out, key, { + value, + configurable: true, + enumerable: false, + }); + } + return out; } /** @@ -153,6 +173,19 @@ function toUndiciRequest( // the DOM `RequestInit`, so it rides along as an extra property. ...(input.body ? { body: input.body, duplex: "half" } : {}), signal: input.signal, + // Everything else the Request carries. Dropping these would silently + // change fetch semantics under a proxy — most visibly `redirect`, where + // a `new Request(url, { redirect: "manual" })` would start following + // redirects instead of surfacing them. + redirect: input.redirect, + cache: input.cache, + credentials: input.credentials, + integrity: input.integrity, + keepalive: input.keepalive, + mode: input.mode, + referrer: input.referrer, + referrerPolicy: input.referrerPolicy, + // An explicit `init` still wins, per the fetch contract. ...init, }, ]; diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs index ec744e07..ed8d5010 100644 --- a/scripts/verify-bundle-externals.test.mjs +++ b/scripts/verify-bundle-externals.test.mjs @@ -9,6 +9,14 @@ import { test, describe } from "node:test"; import assert from "node:assert/strict"; +import { existsSync, readdirSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); import { BUNDLED_CLIENTS, @@ -263,13 +271,38 @@ describe("evaluateClient", () => { }); describe("BUNDLED_CLIENTS", () => { - test("covers every tsup-bundled client", () => { - // `clients/launcher` is plain tsc and emits no bundle, so it is deliberately - // absent. If a fourth bundled client appears, it must be enrolled here. - assert.deepEqual(BUNDLED_CLIENTS.map((c) => c.name).sort(), [ - "cli", - "tui", - "web", - ]); + test("is enrolled from what is actually on disk, not from itself", () => { + // Asserting the hard-coded list equals a hard-coded copy of itself proves + // nothing — adding `clients/foo` with a tsup build would leave it green + // while the new bundle went unchecked. So the expected set is DISCOVERED: + // a client is tsup-bundled iff it ships a tsup config. `clients/launcher` + // is plain tsc, emits no bundle, and is correctly absent. + const clientsDir = path.join(repoRoot, "clients"); + const discovered = readdirSync(clientsDir) + .filter((name) => + readdirSync(path.join(clientsDir, name)).some((f) => + /^tsup(\..+)?\.config\.ts$/.test(f), + ), + ) + .sort(); + + assert.ok(discovered.length > 0, "no tsup-bundled clients discovered"); + assert.deepEqual( + BUNDLED_CLIENTS.map((c) => c.name).sort(), + discovered, + "BUNDLED_CLIENTS is out of sync with the clients that ship a tsup config", + ); + }); + + test("points each client at a config and build dir that exist", () => { + for (const client of BUNDLED_CLIENTS) { + assert.ok( + existsSync(path.join(repoRoot, client.config)), + `${client.name}: missing ${client.config}`, + ); + // The build dir is produced by `npm run build`; only assert the path is + // under the client, since the guard itself reports a missing build. + assert.match(client.build, new RegExp(`^clients/${client.name}/`)); + } }); });