From bf5bbd8cc0312c0bd61744abbaeef5ef6b4b0b00 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 24 Oct 2026 23:02:08 -0400 Subject: [PATCH 1/4] fix(web): guard /api/fetch destinations against link-local and metadata addresses (#2749) Port v1's /fetch guard to v2's POST /api/fetch route, in its own module (core/mcp/remote/node/fetchGuard.ts): - http/https only, on every hop - refuse 169.254.0.0/16 (and its IPv4-mapped IPv6 forms), fe80::/10 and fd00:ec2::254; loopback and RFC 1918 stay reachable - follow redirects manually, re-checking each hop, with v1's per-hop credential/method/body rewrite - on the direct path, validate DNS inside undici's connect.lookup so the socket connects to exactly the addresses that were checked - behind HTTPS_PROXY/HTTP_PROXY the proxy resolves names, so a hostname gets a best-effort local check only; documented in the module and README Refusals answer 403, as on v1. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/project-structure/SKILL.md | 2 +- clients/web/README.md | 15 +- .../core/mcp/remote/node/fetchGuard.test.ts | 380 +++++++++++++++ .../integration/mcp/remote/fetchGuard.test.ts | 104 +++++ .../mcp/remote/server-extra-coverage.test.ts | 53 +++ core/mcp/node/proxyFetch.ts | 7 +- core/mcp/remote/node/fetchGuard.ts | 432 ++++++++++++++++++ core/mcp/remote/node/server.ts | 31 +- 8 files changed, 1013 insertions(+), 11 deletions(-) create mode 100644 clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts create mode 100644 clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts create mode 100644 core/mcp/remote/node/fetchGuard.ts diff --git a/.claude/skills/project-structure/SKILL.md b/.claude/skills/project-structure/SKILL.md index 8928a71a65..7d234aaeb7 100644 --- a/.claude/skills/project-structure/SKILL.md +++ b/.claude/skills/project-structure/SKILL.md @@ -41,7 +41,7 @@ an MCP server, the request/response lifecycle, and a set of state stores. | --- | --- | | `core/mcp/` | `InspectorClient`, transports, state stores, config import, URI templates, subscription/App-elicitation protocol helpers | | `core/mcp/node/` | Node stdio transport factory; `proxyFetch.ts` (the shared HTTPS_PROXY/NO_PROXY fetch) | -| `core/mcp/remote/` | Browser HTTP/SSE transport + remote logger/fetch, and (under `node/`) the Hono backend it talks to | +| `core/mcp/remote/` | Browser HTTP/SSE transport + remote logger/fetch, and (under `node/`) the Hono backend it talks to, plus `fetchGuard.ts` (the `/api/fetch` destination guard) | | `core/mcp/state/` | The stores `core/react/` hooks read | | `core/auth/` | OAuth end to end — providers, discovery, storage, endpoint overrides, scopes, revocation, mid-session recovery — split into isomorphic logic plus `browser/`, `node/` and `remote/` backends | | `core/auth/node/` | Node OAuth storage + loopback callback server, **and** the `SecretStore` backends (keychain / file / memory) and their selection policy | diff --git a/clients/web/README.md b/clients/web/README.md index f69a7ff7f7..6e8238fd8c 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -465,6 +465,19 @@ In every case, exposing the Inspector beyond loopback also means anyone who can ## HTTP proxy support -The web backend connects to remote MCP servers through the shared Node fetch (`core/mcp/node/proxyFetch.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing uses [`undici`](https://www.npmjs.com/package/undici)'s own `fetch` bound to its `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. +The web backend connects to remote MCP servers through the shared Node fetch (`core/mcp/node/proxyFetch.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing uses [`undici`](https://www.npmjs.com/package/undici)'s own `fetch` bound to its `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. (`POST /api/fetch` loads undici either way, on its first request, for the destination guard below.) The proxy sits at the **bottom** of the fetch stack rather than wrapping the transport's fetch, and both undici halves must come from the same copy — see [HTTP proxy support](../cli/README.md#http-proxy-support) in the CLI README for why ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)). + +## Outbound fetch destination guard + +`POST /api/fetch` is how the browser reaches the network for OAuth — discovery, protected-resource metadata, token, client registration, revocation — and some of those URLs come from the server under test. The route therefore refuses destinations a hostile server could use to read the host's cloud credentials ([#2749](https://github.com/modelcontextprotocol/inspector/issues/2749), ported from v1's `/fetch`); the logic is `core/mcp/remote/node/fetchGuard.ts`. + +- **Schemes:** `http:` and `https:` only. +- **Refused addresses:** link-local and instance-metadata only — `169.254.0.0/16` (with its IPv4-mapped IPv6 forms), `fe80::/10` and `fd00:ec2::254`. Loopback and private (RFC 1918) addresses stay reachable, since local and LAN MCP and authorization servers are ordinary targets. +- **Redirects** are followed by the guard, which re-checks every hop, drops credentials on a cross-origin hop, and refuses one that would carry a request body to another origin. +- **DNS** is checked inside the connection's own lookup, so the socket connects to exactly the addresses that were checked — a name cannot resolve differently between check and connect. + +A refusal is answered `403` with an `error` message. **Behind `HTTPS_PROXY` / `HTTP_PROXY` the proxy resolves names, not the Inspector**, so the guard can only check a hostname against local DNS (refusing it if it resolves to a blocked address here, allowing it if it does not resolve here at all) and cannot pin the connection. Scheme, IP-literal and redirect checks still apply. In that configuration the proxy is the network boundary. + +The guard covers the web backend only. The CLI and TUI make their OAuth requests directly from Node, without it. diff --git a/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts b/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts new file mode 100644 index 0000000000..1c9e361144 --- /dev/null +++ b/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts @@ -0,0 +1,380 @@ +/** + * `fetchGuard` — the `/api/fetch` destination guard (#2749). + * + * The block-list, the redirect rewrite and the pinning lookup hook are driven + * directly; the hop loop is driven through an injected `proxyFetch`, which is + * the guard's best-effort (proxied) mode and needs no socket. The direct path, + * which connects through undici, is covered in the integration project + * (`src/test/integration/mcp/remote/fetchGuard.test.ts`). + */ + +import { describe, expect, it, vi } from "vitest"; +import type { LookupAddress } from "node:dns"; +import { + FetchTargetError, + MAX_FETCH_REDIRECTS, + createPinningLookup, + findFetchTargetError, + guardedFetch, + isBlockedAddress, + nextRedirectRequest, + normalizeFetchHeaders, + type FetchHopRequest, + type GuardedFetchInit, + type ResolveHost, +} from "@inspector/core/mcp/remote/node/fetchGuard.js"; + +describe("isBlockedAddress", () => { + it.each([ + "169.254.169.254", + "169.254.0.1", + "::ffff:169.254.169.254", + "::ffff:a9fe:a9fe", + "fe80::1", + "FEBF::1", + "fd00:ec2::254", + ])("blocks %s", (ip) => { + expect(isBlockedAddress(ip)).toBe(true); + }); + + it.each([ + "127.0.0.1", + "10.0.0.1", + "192.168.1.10", + "172.16.0.1", + "169.255.0.1", + "::1", + "::ffff:127.0.0.1", + "::ffff:7f00:1", + "::ffff:not-an-address", + "fec0::1", + "fd00:ec2::253", + "example.com", + ])("allows %s", (ip) => { + expect(isBlockedAddress(ip)).toBe(false); + }); +}); + +describe("normalizeFetchHeaders", () => { + it("keeps string-valued entries only", () => { + expect(normalizeFetchHeaders({ a: "1", b: 2, c: null })).toEqual({ + a: "1", + }); + }); + + it.each([null, "x", ["a"], undefined])("returns {} for %j", (raw) => { + expect(normalizeFetchHeaders(raw)).toEqual({}); + }); +}); + +function hop(overrides: Partial = {}): FetchHopRequest { + return { + url: new URL("https://as.example/token"), + method: "POST", + body: "grant_type=refresh_token", + headers: { + Authorization: "Bearer x", + Cookie: "c=1", + "Content-Type": "application/x-www-form-urlencoded", + Accept: "application/json", + }, + ...overrides, + }; +} + +describe("nextRedirectRequest", () => { + it("rewrites a 302 POST into a bodyless GET and drops body headers", () => { + const next = nextRedirectRequest(hop(), 302, "/elsewhere"); + expect(next.url.href).toBe("https://as.example/elsewhere"); + expect(next.method).toBe("GET"); + expect(next.body).toBeUndefined(); + expect(next.headers).toEqual({ + Authorization: "Bearer x", + Cookie: "c=1", + Accept: "application/json", + }); + }); + + it("keeps a 301 GET as-is", () => { + const next = nextRedirectRequest( + hop({ method: "GET", body: undefined }), + 301, + "/moved", + ); + expect(next.method).toBe("GET"); + expect(next.headers["Content-Type"]).toBe( + "application/x-www-form-urlencoded", + ); + }); + + it("rewrites a 303 PUT but not a 303 HEAD", () => { + expect(nextRedirectRequest(hop({ method: "PUT" }), 303, "/x").method).toBe( + "GET", + ); + expect( + nextRedirectRequest(hop({ method: "HEAD", body: undefined }), 303, "/x") + .method, + ).toBe("HEAD"); + }); + + it("keeps method and body on a same-origin 307", () => { + const next = nextRedirectRequest(hop(), 307, "/again"); + expect(next.method).toBe("POST"); + expect(next.body).toBe("grant_type=refresh_token"); + }); + + it("drops credentials on a cross-origin hop", () => { + const next = nextRedirectRequest( + hop({ method: "GET", body: undefined }), + 302, + "https://other.example/", + ); + expect(next.headers).not.toHaveProperty("Authorization"); + expect(next.headers).not.toHaveProperty("Cookie"); + expect(next.headers.Accept).toBe("application/json"); + }); + + it("refuses a cross-origin hop that would still carry a body", () => { + expect(() => + nextRedirectRequest(hop(), 307, "https://other.example/"), + ).toThrow(FetchTargetError); + }); + + it("does not mutate the current request's headers", () => { + const current = hop(); + nextRedirectRequest(current, 302, "https://other.example/"); + expect(current.headers.Authorization).toBe("Bearer x"); + }); +}); + +describe("findFetchTargetError", () => { + it("finds the error itself or one in its cause chain", () => { + const refused = new FetchTargetError("no"); + expect(findFetchTargetError(refused)).toBe(refused); + expect( + findFetchTargetError(new TypeError("fetch failed", { cause: refused })), + ).toBe(refused); + }); + + it("returns undefined for anything else", () => { + expect(findFetchTargetError(new Error("x"))).toBeUndefined(); + expect(findFetchTargetError("x")).toBeUndefined(); + const cyclic = new Error("a"); + cyclic.cause = cyclic; + expect(findFetchTargetError(cyclic)).toBeUndefined(); + }); +}); + +type LookupResult = { + err: NodeJS.ErrnoException | null; + address: string | LookupAddress[]; + family?: number; +}; + +function runLookup( + resolveHost: ResolveHost, + all: boolean, +): Promise { + const lookup = createPinningLookup(resolveHost); + return new Promise((resolve) => { + lookup("host.test", { all }, (err, address, family) => + resolve({ err, address, family }), + ); + }); +} + +describe("createPinningLookup", () => { + it("hands back every validated address when asked for all", async () => { + const result = await runLookup(async () => ["127.0.0.1", "::1"], true); + expect(result.err).toBeNull(); + expect(result.address).toEqual([ + { address: "127.0.0.1", family: 4 }, + { address: "::1", family: 6 }, + ]); + }); + + it("hands back the first address in the scalar form", async () => { + const result = await runLookup(async () => ["::1", "127.0.0.1"], false); + expect(result).toEqual({ err: null, address: "::1", family: 6 }); + }); + + it("refuses if any resolved address is blocked", async () => { + const result = await runLookup( + async () => ["127.0.0.1", "169.254.169.254"], + true, + ); + expect(result.err).toBeInstanceOf(FetchTargetError); + expect(result.err?.message).toContain("169.254.169.254"); + }); + + it("refuses an empty answer", async () => { + const result = await runLookup(async () => [], true); + expect(result.err).toBeInstanceOf(FetchTargetError); + }); + + it("passes a resolver failure through", async () => { + const failure = Object.assign(new Error("ENOTFOUND"), { + code: "ENOTFOUND", + }); + const result = await runLookup(() => Promise.reject(failure), true); + expect(result.err).toBe(failure); + }); +}); + +function init(overrides: Partial = {}): GuardedFetchInit { + return { + method: "GET", + headers: {}, + body: undefined, + signal: new AbortController().signal, + ...overrides, + }; +} + +function redirect(status: number, location: string | null): Response { + return new Response("moved", { + status, + headers: location === null ? {} : { location }, + }); +} + +/** A proxyFetch answering each call from `responses`, in order. */ +function scripted(...responses: Response[]) { + const proxyFetch = vi.fn(); + for (const res of responses) proxyFetch.mockResolvedValueOnce(res); + return proxyFetch; +} + +const resolvesNowhere: ResolveHost = () => + Promise.reject(new Error("ENOTFOUND")); + +describe("guardedFetch (proxied mode)", () => { + it("passes a non-redirect response straight through", async () => { + const proxyFetch = scripted(new Response("ok")); + const res = await guardedFetch("http://as.example/x", init(), { + proxyFetch, + resolveHost: resolvesNowhere, + }); + expect(await res.text()).toBe("ok"); + expect(proxyFetch).toHaveBeenCalledWith( + "http://as.example/x", + expect.objectContaining({ redirect: "manual", method: "GET" }), + ); + }); + + it("follows a redirect and re-checks the new hop", async () => { + const proxyFetch = scripted( + redirect(302, "http://169.254.169.254/latest/meta-data/"), + ); + await expect( + guardedFetch("http://as.example/x", init(), { + proxyFetch, + resolveHost: resolvesNowhere, + }), + ).rejects.toThrow(FetchTargetError); + expect(proxyFetch).toHaveBeenCalledTimes(1); + }); + + it("follows a permitted redirect to its end", async () => { + const proxyFetch = scripted(redirect(301, "/next"), new Response("landed")); + const res = await guardedFetch("http://as.example/x", init(), { + proxyFetch, + resolveHost: resolvesNowhere, + }); + expect(await res.text()).toBe("landed"); + expect(proxyFetch.mock.calls[1]?.[0]).toBe("http://as.example/next"); + }); + + it("returns a 3xx with no Location as the response", async () => { + const proxyFetch = scripted(redirect(304, null)); + const res = await guardedFetch("http://as.example/x", init(), { + proxyFetch, + resolveHost: resolvesNowhere, + }); + expect(res.status).toBe(304); + }); + + it("gives up after the redirect limit", async () => { + const proxyFetch = vi + .fn() + .mockImplementation(async () => redirect(302, "/loop")); + await expect( + guardedFetch("http://as.example/x", init(), { + proxyFetch, + resolveHost: resolvesNowhere, + }), + ).rejects.toThrow(/more than 5 redirects/); + expect(proxyFetch).toHaveBeenCalledTimes(MAX_FETCH_REDIRECTS + 1); + }); + + it.each(["file:///etc/passwd", "ftp://as.example/", "data:text/plain,x"])( + "refuses the %s scheme before any request", + async (url) => { + const proxyFetch = scripted(); + await expect(guardedFetch(url, init(), { proxyFetch })).rejects.toThrow( + /only http and https/, + ); + expect(proxyFetch).not.toHaveBeenCalled(); + }, + ); + + it.each([ + "http://169.254.169.254/", + "http://[fe80::1]/", + "http://[::ffff:169.254.169.254]/", + "http://2852039166/", + ])("refuses the IP literal in %s", async (url) => { + const proxyFetch = scripted(); + await expect(guardedFetch(url, init(), { proxyFetch })).rejects.toThrow( + FetchTargetError, + ); + expect(proxyFetch).not.toHaveBeenCalled(); + }); + + it("allows loopback and private literals", async () => { + const proxyFetch = scripted(new Response("a"), new Response("b")); + await guardedFetch("http://127.0.0.1:6274/", init(), { proxyFetch }); + await guardedFetch("http://192.168.1.10/", init(), { proxyFetch }); + expect(proxyFetch).toHaveBeenCalledTimes(2); + }); + + it("refuses a hostname that resolves locally to a blocked address", async () => { + const proxyFetch = scripted(); + await expect( + guardedFetch("http://metadata.test/", init(), { + proxyFetch, + resolveHost: async () => ["169.254.169.254"], + }), + ).rejects.toThrow(FetchTargetError); + expect(proxyFetch).not.toHaveBeenCalled(); + }); + + it("allows a hostname that resolves locally to a permitted address", async () => { + const proxyFetch = scripted(new Response("ok")); + await guardedFetch("http://as.example/", init(), { + proxyFetch, + resolveHost: async () => ["10.1.2.3"], + }); + expect(proxyFetch).toHaveBeenCalledTimes(1); + }); + + it("forwards method, headers, body and signal on each hop", async () => { + const proxyFetch = scripted(new Response("ok")); + const controller = new AbortController(); + await guardedFetch( + "http://as.example/token", + init({ + method: "POST", + headers: { "x-probe": "1" }, + body: "a=b", + signal: controller.signal, + }), + { proxyFetch, resolveHost: resolvesNowhere }, + ); + const sent = proxyFetch.mock.calls[0]?.[1]; + expect(sent?.method).toBe("POST"); + expect(sent?.body).toBe("a=b"); + expect(sent?.signal).toBe(controller.signal); + expect(new Headers(sent?.headers).get("x-probe")).toBe("1"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts b/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts new file mode 100644 index 0000000000..4e09e3c778 --- /dev/null +++ b/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts @@ -0,0 +1,104 @@ +/** + * `guardedFetch` on its direct path — undici over the pinning lookup (#2749). + * + * The hostnames here (`*.test`) exist only in the injected resolver, so a + * request that reaches the fixture proves the socket connected to the address + * the guard validated rather than to one it resolved for itself. The proxied + * mode and the pure helpers are covered by the unit suite + * (`src/test/core/mcp/remote/node/fetchGuard.test.ts`). + */ + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { createServer, type Server } from "node:http"; +import { + FetchTargetError, + findFetchTargetError, + guardedFetch, + type GuardedFetchInit, + type ResolveHost, +} from "@inspector/core/mcp/remote/node/fetchGuard.js"; + +let server: Server; +let port: number; +const hits: string[] = []; + +beforeAll(async () => { + server = createServer((req, res) => { + hits.push(`${req.headers.host} ${req.url}`); + if (req.url === "/to-metadata-name") { + res.writeHead(302, { Location: `http://metadata.test:${port}/` }); + res.end(); + return; + } + res.writeHead(200, { "Content-Type": "text/plain" }); + res.end("pinned"); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); + const addr = server.address(); + port = typeof addr === "object" && addr !== null ? addr.port : 0; +}); + +afterAll(async () => { + await new Promise((r) => server.close(() => r())); +}); + +/** Resolves `*.test` names from a fixed table, as a rebinding DNS would. */ +const resolveHost: ResolveHost = async (hostname) => { + if (hostname === "pinned.test") return ["127.0.0.1"]; + if (hostname === "metadata.test") return ["169.254.169.254"]; + throw Object.assign(new Error(`ENOTFOUND ${hostname}`), { + code: "ENOTFOUND", + }); +}; + +function init(): GuardedFetchInit { + return { + method: "GET", + headers: {}, + body: undefined, + signal: AbortSignal.timeout(5_000), + }; +} + +describe("guardedFetch (direct)", () => { + it("connects to the address the guard validated", async () => { + const res = await guardedFetch(`http://pinned.test:${port}/x`, init(), { + resolveHost, + }); + expect(await res.text()).toBe("pinned"); + expect(res).toBeInstanceOf(Response); + expect(hits).toContain(`pinned.test:${port} /x`); + }); + + it("refuses a name that resolves to a metadata address", async () => { + const err: unknown = await guardedFetch( + `http://metadata.test:${port}/`, + init(), + { resolveHost }, + ).catch((e: unknown) => e); + expect(findFetchTargetError(err)).toBeInstanceOf(FetchTargetError); + }); + + it("refuses a redirect to such a name", async () => { + const before = hits.length; + const err: unknown = await guardedFetch( + `http://pinned.test:${port}/to-metadata-name`, + init(), + { resolveHost }, + ).catch((e: unknown) => e); + expect(findFetchTargetError(err)?.message).toContain("169.254.169.254"); + expect(hits.slice(before)).toEqual([ + `pinned.test:${port} /to-metadata-name`, + ]); + }); + + it("surfaces an unresolvable name as an ordinary fetch failure", async () => { + const err: unknown = await guardedFetch( + `http://nowhere.test:${port}/`, + init(), + { resolveHost }, + ).catch((e: unknown) => e); + expect(err).toBeInstanceOf(Error); + expect(findFetchTargetError(err)).toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 71d2a7342b..3ce1ab9444 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -367,6 +367,19 @@ describe("server.ts supplemental coverage", () => { }); const srv = createServer((req, res) => { upstreamHits.push(req.url ?? ""); + if (req.url === "/redirect-to-metadata") { + // A server under test steering OAuth discovery at the IMDS (#2749). + res.writeHead(302, { + Location: "http://169.254.169.254/latest/meta-data/", + }); + res.end(); + return; + } + if (req.url === "/redirect-to-plain") { + res.writeHead(302, { Location: "/plain" }); + res.end(); + return; + } if (req.url === "/not-found-oversize") { // An error status with a body past the budget, chunked so only the // streamed count can catch it. @@ -806,6 +819,46 @@ describe("server.ts supplemental coverage", () => { expect(body.body).toBeUndefined(); }); + // #2749: the destination guard. Refusals are 403, as on v1's `/fetch`. + it.each([ + "http://169.254.169.254/latest/meta-data/", + "http://[fd00:ec2::254]/", + "file:///etc/passwd", + ])("refuses %s with 403 before any request", async (url) => { + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ url }), + }); + expect(res.status).toBe(403); + const body = (await res.json()) as { error: string }; + expect(body.error).toMatch(/^Refusing to fetch/); + }); + + it("refuses a redirect into the metadata range", async () => { + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/redirect-to-metadata` }), + }); + expect(res.status).toBe(403); + const body = (await res.json()) as { error: string }; + expect(body.error).toContain("169.254.169.254"); + expect(upstreamHits).toEqual(["/redirect-to-metadata"]); + }); + + it("follows a permitted redirect to its target", async () => { + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/redirect-to-plain` }), + }); + expect(res.status).toBe(200); + const body = (await res.json()) as { status: number }; + expect(body.status).toBe(200); + expect(upstreamHits).toEqual(["/redirect-to-plain", "/plain"]); + }); + it("relays a body of exactly the response cap", async () => { const res = await fetch(`${h.baseUrl}/api/fetch`, { method: "POST", diff --git a/core/mcp/node/proxyFetch.ts b/core/mcp/node/proxyFetch.ts index 41190be4c8..fe6e5a77aa 100644 --- a/core/mcp/node/proxyFetch.ts +++ b/core/mcp/node/proxyFetch.ts @@ -14,7 +14,7 @@ import type { * bridge a declaration gap rather than a real type difference, and they are kept * to this one module boundary instead of leaking into callers. */ -function asUndiciInit(init: RequestInit | undefined): UndiciRequestInit { +export function asUndiciInit(init: RequestInit | undefined): UndiciRequestInit { return init as UndiciRequestInit; } @@ -77,9 +77,10 @@ const NULL_BODY_STATUSES: ReadonlySet = new Set([ * 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. + * `Response` exposes them as to a consumer. Exported for `fetchGuard.ts`, whose + * direct (unproxied) path calls undici's `fetch` too. */ -function toGlobalResponse(res: UndiciResponse): Response { +export function toGlobalResponse(res: UndiciResponse): Response { const headers = new Headers(); for (const [key, value] of res.headers) headers.append(key, value); const out = new Response( diff --git a/core/mcp/remote/node/fetchGuard.ts b/core/mcp/remote/node/fetchGuard.ts new file mode 100644 index 0000000000..a43bc98c5a --- /dev/null +++ b/core/mcp/remote/node/fetchGuard.ts @@ -0,0 +1,432 @@ +/** + * Destination guard for the web backend's `POST /api/fetch` route (#2749). + * + * The route is the browser's only way out to the network, and it carries the + * web client's OAuth traffic — discovery, protected-resource metadata, token, + * DCR/CIMD, revocation — some of whose URLs come from the server under test. + * Without a guard, a hostile server can point that traffic at the host's + * cloud instance-metadata endpoint and read the answer back through the + * Network log. This is v1's `/fetch` guard (`server/src/proxy-security.ts` + * and `proxy-redirect.ts` on `v1/main`, #1717 / #1732), ported: + * + * - **Scheme:** `http:` and `https:` only, on every hop. + * - **Destinations:** link-local and metadata addresses only — + * `169.254.0.0/16`, its IPv4-mapped IPv6 forms, `fe80::/10` and + * `fd00:ec2::254`. Loopback and RFC 1918 stay reachable on purpose: local + * and LAN MCP servers and authorization servers are legitimate targets. + * - **Redirects:** followed here, with `redirect: "manual"`, so each hop is + * re-validated. The per-hop rewrite the fetch library used to do moves here + * with it ({@link nextRedirectRequest}). + * + * ## DNS: checked and pinned in one step + * + * v1 resolved the name, validated the result, and then pinned an agent to it. + * Here the check lives *inside* the connection's own `lookup` hook (undici's + * `connect.lookup`, handed to `net.connect`): the addresses that hook validates + * are the addresses the socket connects to, so there is no second resolution + * for a rebinding name to answer differently. An IP literal never reaches the + * hook — `net.connect` skips lookup for one — so it is checked before the + * request instead. + * + * ## What the guard can promise behind a proxy + * + * When `HTTPS_PROXY` / `HTTP_PROXY` is set the route goes through + * `createProxyFetch()`, and the **proxy** resolves the name, not us. Scheme, + * redirect re-validation and IP literals are enforced exactly as above. A + * hostname is resolved locally as a best-effort check — refused if it resolves + * to a blocked address here, allowed if it does not resolve here at all (behind + * a corporate proxy local DNS often cannot see external names) — but it cannot + * be pinned, and the proxy is free to resolve it differently. In that + * configuration the proxy is the network boundary, and keeping the metadata + * service out of reach is its job. + * + * ## Out of scope + * + * The CLI and TUI make the same OAuth discovery requests directly from Node, + * without this route. Whether they share the guard is a separate decision. + */ + +import type { LookupAddress } from "node:dns"; +import { lookup as dnsLookup } from "node:dns/promises"; +import { isIP, type LookupFunction } from "node:net"; +import { asUndiciInit, toGlobalResponse } from "../../node/proxyFetch.js"; + +/** Raised when `/api/fetch` is asked to reach a destination it refuses. */ +export class FetchTargetError extends Error { + override name = "FetchTargetError"; +} + +/** Hops followed after the first request before giving up, as in v1. */ +export const MAX_FETCH_REDIRECTS = 5; + +/** The one fetch shape this module calls: a string URL and a full init. */ +type HopFetch = (url: string, init: RequestInit) => Promise; + +/** Resolves a hostname to every address it currently has. */ +export type ResolveHost = (hostname: string) => Promise; + +const resolveWithSystemDns: ResolveHost = async (hostname) => + (await dnsLookup(hostname, { all: true })).map((r) => r.address); + +// --------------------------------------------------------------------------- +// Block-list +// --------------------------------------------------------------------------- + +/** + * Whether `ip` is a link-local or cloud-metadata address that `/api/fetch` + * must never reach. + * + * - `169.254.0.0/16` (IPv4 link-local; includes the `169.254.169.254` IMDS) + * - `::ffff:169.254.x.x` in both its dotted and hex serializations + * - `fe80::/10` (IPv6 link-local) + * - `fd00:ec2::254` (the AWS IPv6 IMDS) + * + * Anything that is not an IP address returns `false`; names are resolved + * before they are checked. + */ +export function isBlockedAddress(ip: string): boolean { + const kind = isIP(ip); + if (kind === 4) { + const [a, b] = ip.split(".").map(Number); + return a === 169 && b === 254; + } + if (kind !== 6) return false; + + const addr = ip.toLowerCase(); + const mapped = /^::ffff:(.+)$/.exec(addr); + if (mapped) { + const tail = mapped[1]; + if (isIP(tail) === 4) return isBlockedAddress(tail); + const hex = /^([0-9a-f]{1,4}):([0-9a-f]{1,4})$/.exec(tail); + if (hex) { + const high = parseInt(hex[1], 16); + const low = parseInt(hex[2], 16); + return isBlockedAddress( + `${high >> 8}.${high & 0xff}.${low >> 8}.${low & 0xff}`, + ); + } + } + if (/^fe[89ab]/.test(addr)) return true; // fe80::/10 + return addr === "fd00:ec2::254"; +} + +/** Throws a {@link FetchTargetError} naming the first blocked address. */ +function assertNoneBlocked(addresses: readonly string[]): void { + const blocked = addresses.find(isBlockedAddress); + if (blocked !== undefined) { + throw new FetchTargetError( + `Refusing to fetch from a link-local or metadata address (${blocked})`, + ); + } +} + +/** `URL.hostname` keeps the brackets around an IPv6 literal. */ +function bareHost(url: URL): string { + return url.hostname.replace(/^\[|\]$/g, ""); +} + +// --------------------------------------------------------------------------- +// The pinning lookup hook +// --------------------------------------------------------------------------- + +/** + * A `net.connect` `lookup` hook that resolves with `resolveHost`, refuses the + * connection if any address is blocked, and otherwise connects to exactly the + * addresses it just validated. + * + * Every address is refused if one is, as in v1, rather than filtering the bad + * ones out: a name answering with a metadata address is hostile, and handing + * the socket the rest would only make that harder to notice. + * + * Both callback shapes are served. Node's `net.connect` runs with + * `autoSelectFamily` on by default, which calls the hook with `{ all: true }` + * and expects an array; the scalar form is the path without it. + */ +export function createPinningLookup(resolveHost: ResolveHost): LookupFunction { + return (hostname, options, callback) => { + resolveHost(hostname).then( + (addresses) => { + try { + assertNoneBlocked(addresses); + } catch (err) { + callback(err as FetchTargetError, ""); + return; + } + const resolved: LookupAddress[] = addresses.map((address) => ({ + address, + family: isIP(address) === 6 ? 6 : 4, + })); + const [first] = resolved; + if (first === undefined) { + callback(new FetchTargetError(`Could not resolve ${hostname}`), ""); + } else if (options.all) { + callback(null, resolved); + } else { + callback(null, first.address, first.family); + } + }, + (err: NodeJS.ErrnoException) => callback(err, ""), + ); + }; +} + +/** + * One undici `fetch` per resolver, each over a pinned `Agent`, so every + * request with the system resolver shares one connection pool. Keyed weakly so + * a resolver a test injects is collected with it. + */ +const directFetches = new WeakMap>(); + +/** undici is loaded lazily, as `proxyFetch.ts` does, on the first request. */ +async function loadDirectFetch(resolveHost: ResolveHost): Promise { + const undici = await import("undici"); + const dispatcher = new undici.Agent({ + connect: { lookup: createPinningLookup(resolveHost) }, + }); + return async (url, init) => + toGlobalResponse( + await undici.fetch(url, { ...asUndiciInit(init), dispatcher }), + ); +} + +function directFetch(resolveHost: ResolveHost): Promise { + let loaded = directFetches.get(resolveHost); + if (loaded === undefined) { + loaded = loadDirectFetch(resolveHost); + directFetches.set(resolveHost, loaded); + } + return loaded; +} + +// --------------------------------------------------------------------------- +// Redirect hops +// --------------------------------------------------------------------------- + +/** Headers that carry credentials and must not cross an origin boundary. */ +const CREDENTIAL_HEADERS: ReadonlySet = new Set([ + "authorization", + "proxy-authorization", + "cookie", + "cookie2", + "www-authenticate", +]); + +/** + * Headers that describe the request body, dropped with it so a rewritten GET + * does not announce a body it no longer has (a stale `Content-Length` would + * leave the upstream waiting for bytes never sent). + */ +const BODY_HEADERS: ReadonlySet = new Set([ + "content-type", + "content-length", + "content-encoding", + "content-language", + "content-location", +]); + +/** One hop's request. */ +export interface FetchHopRequest { + url: URL; + method: string; + body: string | undefined; + headers: Record; +} + +/** + * Copies the route's JSON `headers` into a plain string record, ignoring + * anything that is not a string-valued entry of a plain object. + */ +export function normalizeFetchHeaders(raw: unknown): Record { + const headers: Record = {}; + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { + return headers; + } + for (const [name, value] of Object.entries(raw)) { + if (typeof value === "string") headers[name] = value; + } + return headers; +} + +function withoutHeaders( + headers: Record, + names: ReadonlySet, +): Record { + const kept: Record = {}; + for (const [name, value] of Object.entries(headers)) { + if (!names.has(name.toLowerCase())) kept[name] = value; + } + return kept; +} + +/** + * Whether the Fetch standard rewrites this redirect into a bodyless GET: + * 301/302 do so for POST only, 303 for every method except GET and HEAD. + */ +function rewritesToGet(status: number, method: string): boolean { + const upper = method.toUpperCase(); + if (status === 301 || status === 302) return upper === "POST"; + if (status === 303) return upper !== "GET" && upper !== "HEAD"; + return false; +} + +/** + * The request for the hop after a redirect, following the Fetch standard's + * "HTTP-redirect fetch" rewrite that taking redirects away from the fetch + * library also took away: + * + * - **Credentials stay with their origin.** A hop that changes scheme, host + * or port drops {@link CREDENTIAL_HEADERS}, compared case-insensitively. + * - **Method and body** follow {@link rewritesToGet}; a dropped body takes + * {@link BODY_HEADERS} with it. + * - **A body never crosses an origin.** A cross-origin hop still carrying a + * body after that rewrite — a 307/308 of a POST — is refused, since a + * proxied body here is typically an OAuth form holding a refresh token or + * client secret. + * + * Returns a fresh headers object every hop, so the caller's input is never + * mutated and a header dropped on one hop cannot reappear on a later one. The + * returned URL has still to pass the guard before it is fetched. + */ +export function nextRedirectRequest( + current: FetchHopRequest, + status: number, + location: string, +): FetchHopRequest { + const url = new URL(location, current.url); + let { method, body } = current; + let headers = { ...current.headers }; + const crossOrigin = url.origin !== current.url.origin; + + if (crossOrigin) headers = withoutHeaders(headers, CREDENTIAL_HEADERS); + if (rewritesToGet(status, method)) { + method = "GET"; + body = undefined; + headers = withoutHeaders(headers, BODY_HEADERS); + } + // `!= null`: the body arrives as JSON, so "no body" can be null as well as + // undefined. An empty string is still a body the caller chose to send. + if (crossOrigin && body != null) { + throw new FetchTargetError( + "Refusing to follow a cross-origin redirect for a request with a body", + ); + } + return { url, method, body, headers }; +} + +// --------------------------------------------------------------------------- +// The guarded fetch +// --------------------------------------------------------------------------- + +/** The request `/api/fetch` was asked to make. */ +export interface GuardedFetchInit { + method: string; + headers: Record; + body: string | undefined; + signal: AbortSignal; +} + +export interface GuardedFetchOptions { + /** + * The proxy-aware fetch from `createProxyFetch()`, when a proxy is + * configured. Its presence is what switches the guard to the best-effort + * mode described in the module header. + */ + proxyFetch?: typeof fetch; + /** Overrides the system resolver. For tests. */ + resolveHost?: ResolveHost; +} + +/** + * Checks `url` before it is requested. The scheme and an IP literal are + * checked on every path; a hostname is checked here only behind a proxy, since + * on the direct path the pinning lookup checks it as the socket connects. + */ +async function assertAllowedTarget( + url: URL, + resolveHost: ResolveHost, + viaProxy: boolean, +): Promise { + if (url.protocol !== "http:" && url.protocol !== "https:") { + throw new FetchTargetError( + `Refusing to fetch a ${url.protocol} URL; only http and https are allowed`, + ); + } + const host = bareHost(url); + if (isIP(host)) { + assertNoneBlocked([host]); + return; + } + if (!viaProxy) return; + let addresses: string[]; + try { + addresses = await resolveHost(host); + } catch { + // Not resolvable here is normal behind a proxy; the proxy resolves it. + return; + } + assertNoneBlocked(addresses); +} + +/** + * `fetch(url, init)` for `/api/fetch`, with every hop checked against the + * destination guard. + * + * Rejects with a {@link FetchTargetError} — directly, or as the `cause` of the + * fetch library's own `TypeError` when the pinning lookup refused the + * connection; {@link findFetchTargetError} finds it either way. + */ +export async function guardedFetch( + url: string, + init: GuardedFetchInit, + options: GuardedFetchOptions = {}, +): Promise { + const resolveHost = options.resolveHost ?? resolveWithSystemDns; + const viaProxy = options.proxyFetch !== undefined; + const fetchFn: HopFetch = + options.proxyFetch ?? (await directFetch(resolveHost)); + + let request: FetchHopRequest = { + url: new URL(url), + method: init.method, + body: init.body, + headers: init.headers, + }; + for (let hop = 0; hop <= MAX_FETCH_REDIRECTS; hop++) { + await assertAllowedTarget(request.url, resolveHost, viaProxy); + const res = await fetchFn(request.url.href, { + method: request.method, + headers: new Headers(request.headers), + body: request.body, + signal: init.signal, + redirect: "manual", + }); + const location = res.headers.get("location"); + if (res.status < 300 || res.status >= 400 || location === null) { + return res; + } + // Nobody reads a redirect's body. Not awaited, for the reason the route + // gives on its own stream cancel: `cancel()` may never settle. + void res.body?.cancel().catch(() => {}); + request = nextRedirectRequest(request, res.status, location); + } + throw new FetchTargetError( + `Refusing to follow more than ${MAX_FETCH_REDIRECTS} redirects`, + ); +} + +/** + * The {@link FetchTargetError} behind `err`, if any. undici reports a refused + * connection as `TypeError("fetch failed")` with the lookup's error as its + * `cause`, so the chain is walked rather than `err` alone tested. + */ +export function findFetchTargetError( + err: unknown, +): FetchTargetError | undefined { + let current = err; + // Bounded, in case a cause chain is ever cyclic. + for (let depth = 0; depth < 8 && current instanceof Error; depth++) { + if (current instanceof FetchTargetError) return current; + current = current.cause; + } + return undefined; +} diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index e9cdfbbec4..4b7ca51f4b 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -29,6 +29,11 @@ import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; import { declaresOversizeBody, readCappedText } from "./cappedBody.js"; +import { + findFetchTargetError, + guardedFetch, + normalizeFetchHeaders, +} from "./fetchGuard.js"; import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; import { redactUrlQuery } from "../../fetchTracking.js"; import { isRateLimitError } from "../../rateLimit.js"; @@ -1391,12 +1396,22 @@ export function createRemoteApp( // connect to a server but never authorize against it — and Node's native // `NODE_USE_ENV_PROXY` does not cover them either, being unsupported at // our 22.19 engine floor (#2067). - const res = await (createProxyFetch() ?? fetch)(url, { - method, - headers: new Headers(headers), - body: reqBody, - signal, - }); + // + // And guarded (#2749): some of these URLs come from the server under + // test, so every hop is checked against the link-local / metadata + // block-list, redirects are followed there rather than by the fetch, + // and the direct path pins each connection to the addresses it checked. + // What the guard can and cannot promise behind a proxy is its header. + const res = await guardedFetch( + url, + { + method, + headers: normalizeFetchHeaders(headers), + body: reqBody, + signal, + }, + { proxyFetch: createProxyFetch() }, + ); const resHeaders: Record = {}; res.headers.forEach((v, k) => { @@ -1499,6 +1514,10 @@ export function createRemoteApp( 504, ); } + // A refused destination is the caller's request being declined, not a + // fault in the route, so 403 — the status v1's `/fetch` returns for it. + const refused = findFetchTargetError(err); + if (refused) return c.json({ error: refused.message }, 403); return c.json({ error: msg }, 500); } finally { if (timer !== undefined) clearTimeout(timer); From b4a8df57dc738961d13b6c6fd0c3f368034762f5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 24 Oct 2026 23:32:53 -0400 Subject: [PATCH 2/4] feat: guard OAuth requests in the CLI, TUI and mcpdo too; address review (#2749) Extend the /api/fetch destination guard to every Node client's OAuth traffic, never the configured MCP endpoint: - move the guard to core/mcp/node/fetchGuard.ts and add createGuardedFetch(), a typeof-fetch adapter over it - new InspectorClientEnvironment.oauthFetch: the base of InspectorClient's auth chain, and (via routeOAuthRequests + exemptMcpEndpoint) of the transport chain's non-MCP requests, i.e. the SDK's own discovery on a 401 or a refresh - CLI, TUI and mcpdo fill it, and their standalone OAuth calls (stored-auth refresh, revocation, EMA) use the guarded fetch directly - the web backend's transport routes its OAuth through the guard as well, closing the refresh/discovery path that bypassed /api/fetch Review (Copilot, round 1): - behind a proxy, route through EnvHttpProxyAgent built with the pinning lookup, so a NO_PROXY host's direct connection is pinned too - the proxy-mode DNS preflight now rejects promptly on abort - follow only 301/302/303/307/308; any other 3xx is relayed Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/project-structure/SKILL.md | 4 +- clients/cli/README.md | 6 +- .../clear-stored-auth-for-relogin.test.ts | 11 + .../cli/__tests__/relogin-revocation.test.ts | 11 + .../cli/src/clear-stored-auth-for-relogin.ts | 6 +- clients/cli/src/cli.ts | 18 +- clients/mcpdo/__tests__/authorize.test.ts | 24 +- clients/mcpdo/__tests__/ema.test.ts | 20 +- clients/mcpdo/src/connection/authorize.ts | 4 + clients/mcpdo/src/connection/ema.ts | 8 +- clients/mcpdo/src/daemon/connections.ts | 4 + clients/tui/__tests__/App.test.tsx | 15 +- clients/tui/src/App.tsx | 4 + clients/web/README.md | 14 +- .../test/core/auth/routeOAuthRequests.test.ts | 71 +++ .../src/test/core/mcp/node/fetchGuard.test.ts | 594 ++++++++++++++++++ .../core/mcp/remote/node/fetchGuard.test.ts | 380 ----------- .../mcp/inspectorClient-oauth-fetchFn.test.ts | 103 ++- .../integration/mcp/node/fetchGuard.test.ts | 211 +++++++ .../integration/mcp/node/transport.test.ts | 75 ++- .../integration/mcp/remote/fetchGuard.test.ts | 104 --- core/auth/requestTimeout.ts | 4 +- core/auth/routeOAuthRequests.ts | 38 ++ core/mcp/inspectorClient.ts | 39 +- core/mcp/{remote => }/node/fetchGuard.ts | 251 ++++++-- core/mcp/node/index.ts | 1 + core/mcp/node/transport.ts | 28 +- core/mcp/remote/node/server.ts | 32 +- core/mcp/types.ts | 14 + 29 files changed, 1488 insertions(+), 606 deletions(-) create mode 100644 clients/web/src/test/core/auth/routeOAuthRequests.test.ts create mode 100644 clients/web/src/test/core/mcp/node/fetchGuard.test.ts delete mode 100644 clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts create mode 100644 clients/web/src/test/integration/mcp/node/fetchGuard.test.ts delete mode 100644 clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts create mode 100644 core/auth/routeOAuthRequests.ts rename core/mcp/{remote => }/node/fetchGuard.ts (60%) diff --git a/.claude/skills/project-structure/SKILL.md b/.claude/skills/project-structure/SKILL.md index 7d234aaeb7..9d8317158b 100644 --- a/.claude/skills/project-structure/SKILL.md +++ b/.claude/skills/project-structure/SKILL.md @@ -40,8 +40,8 @@ an MCP server, the request/response lifecycle, and a set of state stores. | Directory | Owns | | --- | --- | | `core/mcp/` | `InspectorClient`, transports, state stores, config import, URI templates, subscription/App-elicitation protocol helpers | -| `core/mcp/node/` | Node stdio transport factory; `proxyFetch.ts` (the shared HTTPS_PROXY/NO_PROXY fetch) | -| `core/mcp/remote/` | Browser HTTP/SSE transport + remote logger/fetch, and (under `node/`) the Hono backend it talks to, plus `fetchGuard.ts` (the `/api/fetch` destination guard) | +| `core/mcp/node/` | Node stdio transport factory; `proxyFetch.ts` (the shared HTTPS_PROXY/NO_PROXY fetch); `fetchGuard.ts` (the OAuth destination guard every Node client and `/api/fetch` use) | +| `core/mcp/remote/` | Browser HTTP/SSE transport + remote logger/fetch, and (under `node/`) the Hono backend it talks to | | `core/mcp/state/` | The stores `core/react/` hooks read | | `core/auth/` | OAuth end to end — providers, discovery, storage, endpoint overrides, scopes, revocation, mid-session recovery — split into isomorphic logic plus `browser/`, `node/` and `remote/` backends | | `core/auth/node/` | Node OAuth storage + loopback callback server, **and** the `SecretStore` backends (keychain / file / memory) and their selection policy | diff --git a/clients/cli/README.md b/clients/cli/README.md index 8e3f588a95..8a3b7364a4 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -90,9 +90,11 @@ The file is the only durable way to give a run its roots: there is no roots flag ### HTTP proxy support -Connections to remote HTTP/SSE servers honor the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. This applies to every Node client — CLI, TUI, and the web backend — with no inspector-specific flag. It also covers **OAuth discovery and token requests**, which run through the same fetch. +Connections to remote HTTP/SSE servers honor the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. This applies to every Node client — CLI, TUI, and the web backend — with no inspector-specific flag. It also covers **OAuth discovery and token requests**. -Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (declared in the **root** manifest only; `^8.x` requires Node `>= 22.19.0`, the Inspector's supported floor). It is imported lazily on the first request and only when a proxy variable is set, so runs without a proxy configured pay no cost. +OAuth requests go through the destination guard rather than the plain proxied fetch: it reads the same variables, refuses link-local and cloud-metadata targets, and pins each direct connection to the addresses it checked ([#2749](https://github.com/modelcontextprotocol/inspector/issues/2749)). What it can and cannot promise behind a proxy is in [Outbound fetch destination guard](../web/README.md#outbound-fetch-destination-guard). + +Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (declared in the **root** manifest only; `^8.x` requires Node `>= 22.19.0`, the Inspector's supported floor). It is imported lazily on the first request and, for MCP traffic, only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. OAuth requests load it either way, for the destination guard. **Both halves of the pair come from userland undici — its `fetch` _and_ its `EnvHttpProxyAgent` ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)).** The obvious-looking alternative is to hand the agent to Node's built-in `fetch` as a `dispatcher`, which is what v2.0.0–2.3.0 did. That couples two _different copies_ of undici at the dispatcher handler interface, and that interface is not stable across majors: Node 22 embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8 agent handed a built-in undici 7 handler is rejected outright — `fetch failed: invalid onRequestStart method` — and the request never leaves the process. Keeping both sides inside one copy is what makes proxying work unchanged from the Node 22.19 floor through Node 26. (Node's own `NODE_USE_ENV_PROXY` is not a substitute: it is unsupported at our 22.19 engine floor.) diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 47d171c45f..39aa300691 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -9,6 +9,17 @@ import { import { MIN_REVOCATION_REQUEST_BUDGET_MS } from "@inspector/core/auth/revocation.js"; import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; +// The OAuth destination guard (#2749) calls undici directly, past the +// `globalThis.fetch` spy these tests fake the authorization server with, so +// route it back through the spy. The guard itself is tested in clients/web. +vi.mock("@inspector/core/mcp/node/fetchGuard.js", async (importOriginal) => ({ + ...(await importOriginal< + typeof import("@inspector/core/mcp/node/fetchGuard.js") + >()), + createGuardedFetch: (): typeof fetch => (input, init) => + globalThis.fetch(input, init), +})); + const AS_ISSUER = "https://as.example.com"; describe("clearStoredAuthForRelogin", () => { diff --git a/clients/cli/__tests__/relogin-revocation.test.ts b/clients/cli/__tests__/relogin-revocation.test.ts index 629607f487..3355f7c60a 100644 --- a/clients/cli/__tests__/relogin-revocation.test.ts +++ b/clients/cli/__tests__/relogin-revocation.test.ts @@ -20,6 +20,17 @@ import * as path from "node:path"; import { resetNodeOAuthStorageCache } from "@inspector/core/auth/node/storage-node.js"; import { runCli } from "./helpers/cli-runner.js"; +// The OAuth destination guard (#2749) calls undici directly, past the +// `globalThis.fetch` spy these tests fake the authorization server with, so +// route it back through the spy. The guard itself is tested in clients/web. +vi.mock("@inspector/core/mcp/node/fetchGuard.js", async (importOriginal) => ({ + ...(await importOriginal< + typeof import("@inspector/core/mcp/node/fetchGuard.js") + >()), + createGuardedFetch: (): typeof fetch => (input, init) => + globalThis.fetch(input, init), +})); + const SERVER_URL = "https://example.com/mcp"; const AS_ISSUER = "https://as.example.com"; const REVOKE_URL = `${AS_ISSUER}/revoke`; diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index a67695d9d3..fd4eb50b18 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -10,7 +10,7 @@ import { type OAuthRevocationPlan, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; -import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; +import { createGuardedFetch } from "@inspector/core/mcp/node/fetchGuard.js"; /** Same canonicalisation as CLI `normalizeServerUrl` (avoid cycles). */ function normalizeServerUrl(serverUrl: string): string { @@ -113,7 +113,9 @@ async function sendPlans( plans: OAuthRevocationPlan[], budgetMs: number, ): Promise { - const fetchFn = createProxyFetch() ?? fetch; + // Revocation endpoints come from authorization-server metadata, so they get + // the same destination guard as the rest of OAuth (#2749); proxy-aware too. + const fetchFn = createGuardedFetch(); // Monotonic and epsilon-floored for the same reasons as the shared deadline // inside `executeOAuthRevocation` — see MIN_REVOCATION_REQUEST_BUDGET_MS. const deadlineAt = performance.now() + budgetMs; diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index b8c23ff27e..6a2abd1196 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -38,6 +38,7 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { cleanRoots } from "@inspector/core/mcp/serverList.js"; import { UI_EXTENSION_KEY } from "@inspector/core/mcp/extensions.js"; import { + createGuardedFetch, createProxyFetch, createTransportNode, loadServerEntries, @@ -206,6 +207,10 @@ async function callMethod( // also run through `environment.fetch`, are proxied too (#2067). Undefined // when no proxy env var is set, which leaves the built-in fetch in place. fetch: createProxyFetch(), + // OAuth on its own base, which refuses link-local and cloud-metadata + // destinations a hostile server could steer discovery at (#2749). It is + // proxy-aware itself, so the proxy still covers OAuth. + oauthFetch: createGuardedFetch(), }; const redirectUrlProvider = new MutableRedirectUrlProvider(); // Disarmed until the CLI-owned interactive OAuth flow runs — SDK `auth()` @@ -527,18 +532,19 @@ export async function refreshStoredAuthToken( // `serverMetadata` still cannot refresh against an authorization server that // publishes RFC 8414 metadata at the OIDC well-known path (Copilot). // - // Built over `createProxyFetch()` for the same reason `environment.fetch` is - // (#2067): this whole function runs outside `InspectorClient`, so nothing - // else puts a proxy under it, and a server reachable only through - // `HTTPS_PROXY` would otherwise be probed directly. The same fetch is handed - // to the token request below, so neither leg bypasses the proxy (Copilot). + // Built over `createGuardedFetch()` for the same reason `environment.oauthFetch` + // is: this whole function runs outside `InspectorClient`, so nothing else + // puts the destination guard (#2749) or a proxy (#2067) under it — the guard + // is proxy-aware, so a server reachable only through `HTTPS_PROXY` is still + // reached through it. The same fetch is handed to the token request below, + // so neither leg bypasses either (Copilot). // #2319: this path runs outside `InspectorClient`, so nothing else bounds it // — the discovery and the token request below would otherwise hang forever // against an authorization server that accepts the connection and never // answers. Innermost, so the compat wrapper's own probe requests inherit the // deadline too. const storedAuthFetch = withRfc8414OidcCompat( - withOAuthRequestTimeout(createProxyFetch() ?? fetch), + withOAuthRequestTimeout(createGuardedFetch()), ); const discover: typeof discoverAuthorizationServerMetadata = deps.discover ?? diff --git a/clients/mcpdo/__tests__/authorize.test.ts b/clients/mcpdo/__tests__/authorize.test.ts index 89b92d15cb..7dda1848cc 100644 --- a/clients/mcpdo/__tests__/authorize.test.ts +++ b/clients/mcpdo/__tests__/authorize.test.ts @@ -7,6 +7,7 @@ const disconnectSpy = vi.fn().mockResolvedValue(undefined); const navigationSpy = vi.fn(); const clientCtorSpy = vi.fn(); const createProxyFetchSpy = vi.fn<() => typeof fetch | undefined>(); +const createGuardedFetchSpy = vi.fn<() => typeof fetch>(); vi.mock("@inspector/core/cli/cliOAuth.js", () => ({ connectInspectorWithOAuth: (...args: unknown[]) => connectSpy(...args), @@ -32,7 +33,11 @@ vi.mock("@inspector/core/mcp/index.js", () => ({ vi.mock("@inspector/core/mcp/node/index.js", async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, createProxyFetch: () => createProxyFetchSpy() }; + return { + ...actual, + createProxyFetch: () => createProxyFetchSpy(), + createGuardedFetch: () => createGuardedFetchSpy(), + }; }); vi.mock("@inspector/core/client/runner.js", async (importOriginal) => { @@ -52,6 +57,7 @@ describe("authorizeInFrontend", () => { navigationSpy.mockClear(); clientCtorSpy.mockClear(); createProxyFetchSpy.mockReset(); + createGuardedFetchSpy.mockReset(); }); it("installs the proxy-aware fetch on the client environment (#2642)", async () => { @@ -73,6 +79,22 @@ describe("authorizeInFrontend", () => { expect(options.environment.fetch).toBe(proxyFetch); }); + it("installs the guarded fetch as the OAuth base (#2749)", async () => { + connectSpy.mockResolvedValue(undefined); + const guardedFetch: typeof fetch = vi.fn(); // a stand-in, never called + createGuardedFetchSpy.mockReturnValue(guardedFetch); + const { authorizeInFrontend } = + await import("../src/connection/authorize.js"); + await authorizeInFrontend( + { type: "streamable-http", url: "https://example.com/mcp" }, + undefined, + ); + const options = clientCtorSpy.mock.calls[0]?.[1] as { + environment: { oauthFetch?: typeof fetch }; + }; + expect(options.environment.oauthFetch).toBe(guardedFetch); + }); + it("names the CLI's timeout knobs, not the web's Server Settings (#2615)", async () => { connectSpy.mockResolvedValue(undefined); const { authorizeInFrontend } = diff --git a/clients/mcpdo/__tests__/ema.test.ts b/clients/mcpdo/__tests__/ema.test.ts index 081f3f7eda..8ba1372c2b 100644 --- a/clients/mcpdo/__tests__/ema.test.ts +++ b/clients/mcpdo/__tests__/ema.test.ts @@ -11,10 +11,10 @@ import { const runRunnerInteractiveOAuth = vi.fn(); const startIdpOidcAuthorization = vi.fn(); const completeIdpOidcAuthorization = vi.fn(); -const createProxyFetch = vi.fn<() => typeof fetch | undefined>(); +const createGuardedFetch = vi.fn<() => typeof fetch>(); -vi.mock("@inspector/core/mcp/node/proxyFetch.js", () => ({ - createProxyFetch: () => createProxyFetch(), +vi.mock("@inspector/core/mcp/node/fetchGuard.js", () => ({ + createGuardedFetch: () => createGuardedFetch(), })); vi.mock("@inspector/core/auth/node/index.js", async (importOriginal) => { @@ -62,7 +62,7 @@ describe("mcpdo ema helpers", () => { runRunnerInteractiveOAuth.mockReset(); startIdpOidcAuthorization.mockReset(); completeIdpOidcAuthorization.mockReset(); - createProxyFetch.mockReset(); + createGuardedFetch.mockReset(); }); afterEach(() => { @@ -264,10 +264,12 @@ describe("mcpdo ema helpers", () => { ); }); - it("emaLogin routes IdP discovery and the code exchange through the proxy-aware fetch (#2642)", async () => { + // The guarded fetch is proxy-aware itself (#2642), and refuses link-local and + // cloud-metadata destinations (#2749). + it("emaLogin routes IdP discovery and the code exchange through the guarded fetch", async () => { writeClientConfig(emaClientConfig()); - const proxyFetch: typeof fetch = vi.fn(); // a stand-in, never called - createProxyFetch.mockReturnValue(proxyFetch); + const guardedFetch: typeof fetch = vi.fn(); // a stand-in, never called + createGuardedFetch.mockReturnValue(guardedFetch); startIdpOidcAuthorization.mockResolvedValue({ authorizationUrl: new URL("https://idp.example.com/authorize"), }); @@ -296,10 +298,10 @@ describe("mcpdo ema helpers", () => { process.stderr.write = originalWrite; } expect(startIdpOidcAuthorization).toHaveBeenCalledWith( - expect.objectContaining({ fetchFn: proxyFetch }), + expect.objectContaining({ fetchFn: guardedFetch }), ); expect(completeIdpOidcAuthorization).toHaveBeenCalledWith( - expect.objectContaining({ fetchFn: proxyFetch }), + expect.objectContaining({ fetchFn: guardedFetch }), ); }); diff --git a/clients/mcpdo/src/connection/authorize.ts b/clients/mcpdo/src/connection/authorize.ts index a999212b76..490d0185f1 100644 --- a/clients/mcpdo/src/connection/authorize.ts +++ b/clients/mcpdo/src/connection/authorize.ts @@ -12,6 +12,7 @@ import { } from "@inspector/core/client/runner.js"; import { InspectorClient } from "@inspector/core/mcp/index.js"; import { + createGuardedFetch, createProxyFetch, createTransportNode, } from "@inspector/core/mcp/node/index.js"; @@ -65,6 +66,9 @@ export async function authorizeInFrontend( // at the bottom of the stack, for OAuth discovery and token requests to // reach it. Undefined when no proxy variable is set. fetch: createProxyFetch(), + // OAuth on the destination-guarded base (#2749), which refuses link-local + // and cloud-metadata targets; proxy-aware itself. + oauthFetch: createGuardedFetch(), }; const redirectUrlProvider = new MutableRedirectUrlProvider(); const callbackUrlConfig = parseRunnerOAuthCallbackUrl( diff --git a/clients/mcpdo/src/connection/ema.ts b/clients/mcpdo/src/connection/ema.ts index 88e7a5353b..0751217d56 100644 --- a/clients/mcpdo/src/connection/ema.ts +++ b/clients/mcpdo/src/connection/ema.ts @@ -22,7 +22,7 @@ import { parseRunnerOAuthCallbackUrl, } from "@inspector/core/auth/node/runner-oauth-callback.js"; import { getClientConfigFilePath } from "@inspector/core/client/index.js"; -import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; +import { createGuardedFetch } from "@inspector/core/mcp/node/fetchGuard.js"; import { loadRunnerClientConfig } from "@inspector/core/client/runner.js"; import type { EnterpriseManagedAuthIdpConfig } from "@inspector/core/client/types.js"; import { createCliOAuthNavigation } from "@inspector/core/cli/cli-oauth-navigation.js"; @@ -233,9 +233,9 @@ export async function runEmaIdpInteractiveFlow( redirectUrlProvider.redirectUrl = formatRunnerOAuthRedirectUrl(callbackUrlConfig); // IdP discovery and the code exchange run outside `InspectorClient`, so - // nothing else puts HTTPS_PROXY / HTTP_PROXY under them (#2642). Undefined - // when no proxy variable is set, which leaves the built-in fetch in place. - const fetchFn = createProxyFetch(); + // nothing else puts HTTPS_PROXY / HTTP_PROXY (#2642) or the OAuth + // destination guard (#2749) under them. The guarded fetch is both. + const fetchFn = createGuardedFetch(); // Adapter over the server-bound runner-interactive-OAuth surface: EMA leg 1 // is server-less, so authenticate/completeOAuthFlow map straight onto the diff --git a/clients/mcpdo/src/daemon/connections.ts b/clients/mcpdo/src/daemon/connections.ts index 77809afc7e..a2864a1985 100644 --- a/clients/mcpdo/src/daemon/connections.ts +++ b/clients/mcpdo/src/daemon/connections.ts @@ -10,6 +10,7 @@ import { type MCPServerConfig, } from "@inspector/core/mcp/types.js"; import { + createGuardedFetch, createProxyFetch, createTransportNode, } from "@inspector/core/mcp/node/index.js"; @@ -826,6 +827,9 @@ async function createConnectionClient( // `createProxyFetch()` fallback is never reached — leave this out and the // daemon connects direct. Undefined when no proxy variable is set. fetch: createProxyFetch(), + // OAuth on the destination-guarded base (#2749), which refuses link-local + // and cloud-metadata targets; proxy-aware itself. + oauthFetch: createGuardedFetch(), }; const redirectUrlProvider = new MutableRedirectUrlProvider(); if (isOAuthCapableServerConfig(serverConfig)) { diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index a7271ab249..0d15556b35 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -144,7 +144,7 @@ const h = vi.hoisted(() => { // (#2615). opts?: { defaultMetadata?: unknown; - environment?: { fetch?: unknown }; + environment?: { fetch?: unknown; oauthFetch?: unknown }; connectionTimeoutRemedy?: string; }; }> = []; @@ -152,6 +152,9 @@ const h = vi.hoisted(() => { // 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 }; + // What the mocked `createGuardedFetch` returns: a fixed sentinel, so a test + // can assert it landed in `environment.oauthFetch` (#2749). + const guardedFetchSentinel: typeof fetch = async () => new Response(""); const fireClientEvent = (event: string, detail?: unknown) => { clientEvents.get(event)?.forEach((e) => e.fn({ detail })); }; @@ -259,6 +262,7 @@ const h = vi.hoisted(() => { return { ctrl, proxyFetchState, + guardedFetchSentinel, connect, disconnect, openUrl, @@ -337,6 +341,7 @@ vi.mock("@inspector/core/mcp/node/index.js", () => ({ // test can point `h.proxyFetchState.current` at a sentinel to exercise the // other branch. createProxyFetch: vi.fn(() => h.proxyFetchState.current), + createGuardedFetch: vi.fn(() => h.guardedFetchSentinel), })); vi.mock("@inspector/core/react/useInspectorClient.js", () => ({ useInspectorClient: h.useInspectorClient, @@ -2191,6 +2196,14 @@ describe("App (OAuth result branches)", () => { expect(h.clientInstances[0]!.opts?.environment?.fetch).toBeUndefined(); }); + it("installs the guarded fetch as environment.oauthFetch (#2749)", async () => { + await mount(oneHttp()); + expect(h.clientInstances.length).toBeGreaterThan(0); + expect(h.clientInstances[0]!.opts?.environment?.oauthFetch).toBe( + h.guardedFetchSentinel, + ); + }); + 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/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 43c1e740ed..1960a222ca 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -38,6 +38,7 @@ import { ResourceSubscriptionsState, } from "@inspector/core/mcp/state/index.js"; import { + createGuardedFetch, createProxyFetch, createTransportNode, } from "@inspector/core/mcp/node/index.js"; @@ -401,6 +402,9 @@ function App({ // over it and OAuth requests are proxied too (#2067). Undefined when // no proxy env var is set. fetch: createProxyFetch(), + // OAuth on the destination-guarded base (#2749), which refuses + // link-local and cloud-metadata targets; proxy-aware itself. + oauthFetch: createGuardedFetch(), }; // Per-server default `_meta` is already a JSON object (#1910) — no // pair-array flattening left to do; `{}` means "no defaults". diff --git a/clients/web/README.md b/clients/web/README.md index 6e8238fd8c..a89e30a33a 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -465,19 +465,23 @@ In every case, exposing the Inspector beyond loopback also means anyone who can ## HTTP proxy support -The web backend connects to remote MCP servers through the shared Node fetch (`core/mcp/node/proxyFetch.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing uses [`undici`](https://www.npmjs.com/package/undici)'s own `fetch` bound to its `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. (`POST /api/fetch` loads undici either way, on its first request, for the destination guard below.) +The web backend connects to remote MCP servers through the shared Node fetch (`core/mcp/node/proxyFetch.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing uses [`undici`](https://www.npmjs.com/package/undici)'s own `fetch` bound to its `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. (OAuth requests load undici either way, on their first request, for the destination guard below.) The proxy sits at the **bottom** of the fetch stack rather than wrapping the transport's fetch, and both undici halves must come from the same copy — see [HTTP proxy support](../cli/README.md#http-proxy-support) in the CLI README for why ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)). ## Outbound fetch destination guard -`POST /api/fetch` is how the browser reaches the network for OAuth — discovery, protected-resource metadata, token, client registration, revocation — and some of those URLs come from the server under test. The route therefore refuses destinations a hostile server could use to read the host's cloud credentials ([#2749](https://github.com/modelcontextprotocol/inspector/issues/2749), ported from v1's `/fetch`); the logic is `core/mcp/remote/node/fetchGuard.ts`. +OAuth discovery, protected-resource metadata, token, client registration and revocation URLs come partly from the server under test. Every Inspector client therefore sends its OAuth requests through a destination guard that refuses targets a hostile server could use to read the host's cloud credentials ([#2749](https://github.com/modelcontextprotocol/inspector/issues/2749), ported from v1's `/fetch`). The logic is `core/mcp/node/fetchGuard.ts`. - **Schemes:** `http:` and `https:` only. - **Refused addresses:** link-local and instance-metadata only — `169.254.0.0/16` (with its IPv4-mapped IPv6 forms), `fe80::/10` and `fd00:ec2::254`. Loopback and private (RFC 1918) addresses stay reachable, since local and LAN MCP and authorization servers are ordinary targets. -- **Redirects** are followed by the guard, which re-checks every hop, drops credentials on a cross-origin hop, and refuses one that would carry a request body to another origin. +- **Redirects** are followed by the guard, which re-checks every hop, drops credentials on a cross-origin hop, and refuses one that would carry a request body to another origin. Only 301, 302, 303, 307 and 308 are followed. - **DNS** is checked inside the connection's own lookup, so the socket connects to exactly the addresses that were checked — a name cannot resolve differently between check and connect. -A refusal is answered `403` with an `error` message. **Behind `HTTPS_PROXY` / `HTTP_PROXY` the proxy resolves names, not the Inspector**, so the guard can only check a hostname against local DNS (refusing it if it resolves to a blocked address here, allowing it if it does not resolve here at all) and cannot pin the connection. Scheme, IP-literal and redirect checks still apply. In that configuration the proxy is the network boundary. +**What it covers.** OAuth traffic only — never the MCP endpoint you configured: -The guard covers the web backend only. The CLI and TUI make their OAuth requests directly from Node, without it. +- the browser's OAuth, through `POST /api/fetch` (a refusal is answered `403` with an `error` message); +- the CLI's, TUI's and mcpdo's OAuth, through `InspectorClientEnvironment.oauthFetch`, including the discovery the SDK runs from inside the transport on a `401` or a token refresh; +- the web backend's own transport, which refreshes with the tokens the browser hands it. + +**Behind `HTTPS_PROXY` / `HTTP_PROXY`** the guard routes through undici's `EnvHttpProxyAgent`, so a `NO_PROXY` host is still connected directly and pinned. A proxied host is resolved by the proxy, not by the Inspector, so the guard can only check its name against local DNS — refusing it if it resolves to a blocked address here, allowing it if it does not resolve here at all — and cannot pin the connection. Scheme, IP-literal and redirect checks still apply. For proxied traffic the proxy is the network boundary. diff --git a/clients/web/src/test/core/auth/routeOAuthRequests.test.ts b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts new file mode 100644 index 0000000000..6a033da04a --- /dev/null +++ b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts @@ -0,0 +1,71 @@ +/** + * `routeOAuthRequests` — MCP traffic to one fetch, OAuth work to another + * (#2749). + */ + +import { describe, expect, it, vi } from "vitest"; +import { routeOAuthRequests } from "@inspector/core/auth/routeOAuthRequests.js"; +import { exemptMcpEndpoint } from "@inspector/core/auth/requestTimeout.js"; + +function fetches() { + const mcp = vi.fn().mockResolvedValue(new Response("mcp")); + const oauth = vi.fn().mockResolvedValue(new Response("oauth")); + return { mcp, oauth }; +} + +describe("routeOAuthRequests", () => { + it("routes by the predicate, handing each fetch the original arguments", async () => { + const { mcp, oauth } = fetches(); + const isMcp = vi.fn((url: string) => url.endsWith("/mcp")); + const routed = routeOAuthRequests(mcp, oauth, isMcp); + + const mcpInit = { method: "POST", body: "{}" }; + expect(await (await routed("http://s.example/mcp", mcpInit)).text()).toBe( + "mcp", + ); + expect(mcp).toHaveBeenCalledWith("http://s.example/mcp", mcpInit); + + const url = new URL( + "http://as.example/.well-known/oauth-protected-resource", + ); + expect(await (await routed(url)).text()).toBe("oauth"); + expect(oauth).toHaveBeenCalledWith(url, undefined); + expect(isMcp).toHaveBeenLastCalledWith(url.href, expect.any(Headers)); + }); + + it("hands the predicate a Request's URL and headers", async () => { + const { mcp, oauth } = fetches(); + const isMcp = vi.fn<(url: string, headers: Headers) => boolean>( + () => false, + ); + const req = new Request("http://as.example/token", { + headers: { "x-probe": "1" }, + }); + await routeOAuthRequests(mcp, oauth, isMcp)(req); + const [seenUrl, seenHeaders] = isMcp.mock.calls[0] ?? []; + expect(seenUrl).toBe("http://as.example/token"); + expect(seenHeaders?.get("x-probe")).toBe("1"); + }); + + it("sends a same-path OAuth form to OAuth under exemptMcpEndpoint", async () => { + const { mcp, oauth } = fetches(); + const routed = routeOAuthRequests( + mcp, + oauth, + exemptMcpEndpoint(() => "http://s.example/mcp"), + ); + await routed("http://s.example/mcp", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + await routed("http://s.example/mcp", { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: "grant_type=refresh_token", + }); + await routed("http://as.example/token"); + expect(mcp).toHaveBeenCalledTimes(1); + expect(oauth).toHaveBeenCalledTimes(2); + }); +}); diff --git a/clients/web/src/test/core/mcp/node/fetchGuard.test.ts b/clients/web/src/test/core/mcp/node/fetchGuard.test.ts new file mode 100644 index 0000000000..194d66c4fd --- /dev/null +++ b/clients/web/src/test/core/mcp/node/fetchGuard.test.ts @@ -0,0 +1,594 @@ +/** + * `fetchGuard` — the OAuth destination guard (#2749). + * + * The block-list, the redirect rewrite and the pinning lookup hook are driven + * directly; the hop loop and the `typeof fetch` adapter are driven through an + * injected `hopFetch`, so no socket is needed. The undici path — direct and + * through `EnvHttpProxyAgent` — is covered in the integration project + * (`src/test/integration/mcp/node/fetchGuard.test.ts`). + */ + +import { describe, expect, it, vi } from "vitest"; +import type { LookupAddress } from "node:dns"; +import { + FetchTargetError, + MAX_FETCH_REDIRECTS, + createGuardedFetch, + createPinningLookup, + findFetchTargetError, + guardedFetch, + isBlockedAddress, + nextRedirectRequest, + normalizeFetchHeaders, + type FetchHopRequest, + type GuardedFetchInit, + type HopFetch, + type ResolveHost, +} from "@inspector/core/mcp/node/fetchGuard.js"; + +describe("isBlockedAddress", () => { + it.each([ + "169.254.169.254", + "169.254.0.1", + "::ffff:169.254.169.254", + "::ffff:a9fe:a9fe", + "fe80::1", + "FEBF::1", + "fd00:ec2::254", + ])("blocks %s", (ip) => { + expect(isBlockedAddress(ip)).toBe(true); + }); + + it.each([ + "127.0.0.1", + "10.0.0.1", + "192.168.1.10", + "172.16.0.1", + "169.255.0.1", + "::1", + "::ffff:127.0.0.1", + "::ffff:7f00:1", + "::ffff:not-an-address", + "fec0::1", + "fd00:ec2::253", + "example.com", + ])("allows %s", (ip) => { + expect(isBlockedAddress(ip)).toBe(false); + }); +}); + +describe("normalizeFetchHeaders", () => { + it("keeps string-valued entries only", () => { + expect(normalizeFetchHeaders({ a: "1", b: 2, c: null })).toEqual({ + a: "1", + }); + }); + + it.each([null, "x", ["a"], undefined])("returns {} for %j", (raw) => { + expect(normalizeFetchHeaders(raw)).toEqual({}); + }); +}); + +function hop(overrides: Partial = {}): FetchHopRequest { + return { + url: new URL("https://as.example/token"), + method: "POST", + body: "grant_type=refresh_token", + headers: { + Authorization: "Bearer x", + Cookie: "c=1", + "Content-Type": "application/x-www-form-urlencoded", + Accept: "application/json", + }, + ...overrides, + }; +} + +describe("nextRedirectRequest", () => { + it("rewrites a 302 POST into a bodyless GET and drops body headers", () => { + const next = nextRedirectRequest(hop(), 302, "/elsewhere"); + expect(next.url.href).toBe("https://as.example/elsewhere"); + expect(next.method).toBe("GET"); + expect(next.body).toBeUndefined(); + expect(next.headers).toEqual({ + Authorization: "Bearer x", + Cookie: "c=1", + Accept: "application/json", + }); + }); + + it("keeps a 301 GET as-is", () => { + const next = nextRedirectRequest( + hop({ method: "GET", body: undefined }), + 301, + "/moved", + ); + expect(next.method).toBe("GET"); + expect(next.headers["Content-Type"]).toBe( + "application/x-www-form-urlencoded", + ); + }); + + it("rewrites a 303 PUT but not a 303 HEAD", () => { + expect(nextRedirectRequest(hop({ method: "PUT" }), 303, "/x").method).toBe( + "GET", + ); + expect( + nextRedirectRequest(hop({ method: "HEAD", body: undefined }), 303, "/x") + .method, + ).toBe("HEAD"); + }); + + it("keeps method and body on a same-origin 307", () => { + const next = nextRedirectRequest(hop(), 307, "/again"); + expect(next.method).toBe("POST"); + expect(next.body).toBe("grant_type=refresh_token"); + }); + + it("drops credentials on a cross-origin hop", () => { + const next = nextRedirectRequest( + hop({ method: "GET", body: undefined }), + 302, + "https://other.example/", + ); + expect(next.headers).not.toHaveProperty("Authorization"); + expect(next.headers).not.toHaveProperty("Cookie"); + expect(next.headers.Accept).toBe("application/json"); + }); + + it("refuses a cross-origin hop that would still carry a body", () => { + expect(() => + nextRedirectRequest(hop(), 307, "https://other.example/"), + ).toThrow(FetchTargetError); + }); + + it("does not mutate the current request's headers", () => { + const current = hop(); + nextRedirectRequest(current, 302, "https://other.example/"); + expect(current.headers.Authorization).toBe("Bearer x"); + }); +}); + +describe("findFetchTargetError", () => { + it("finds the error itself or one in its cause chain", () => { + const refused = new FetchTargetError("no"); + expect(findFetchTargetError(refused)).toBe(refused); + expect( + findFetchTargetError(new TypeError("fetch failed", { cause: refused })), + ).toBe(refused); + }); + + it("returns undefined for anything else", () => { + expect(findFetchTargetError(new Error("x"))).toBeUndefined(); + expect(findFetchTargetError("x")).toBeUndefined(); + const cyclic = new Error("a"); + cyclic.cause = cyclic; + expect(findFetchTargetError(cyclic)).toBeUndefined(); + }); +}); + +type LookupResult = { + err: NodeJS.ErrnoException | null; + address: string | LookupAddress[]; + family?: number; +}; + +function runLookup( + resolveHost: ResolveHost, + all: boolean, +): Promise { + const lookup = createPinningLookup(resolveHost); + return new Promise((resolve) => { + lookup( + "host.test", + { all }, + ( + err: NodeJS.ErrnoException | null, + address: string | LookupAddress[], + family?: number, + ) => resolve({ err, address, family }), + ); + }); +} + +describe("createPinningLookup", () => { + it("hands back every validated address when asked for all", async () => { + const result = await runLookup(async () => ["127.0.0.1", "::1"], true); + expect(result.err).toBeNull(); + expect(result.address).toEqual([ + { address: "127.0.0.1", family: 4 }, + { address: "::1", family: 6 }, + ]); + }); + + it("hands back the first address in the scalar form", async () => { + const result = await runLookup(async () => ["::1", "127.0.0.1"], false); + expect(result).toEqual({ err: null, address: "::1", family: 6 }); + }); + + it("refuses if any resolved address is blocked", async () => { + const result = await runLookup( + async () => ["127.0.0.1", "169.254.169.254"], + true, + ); + expect(result.err).toBeInstanceOf(FetchTargetError); + expect(result.err?.message).toContain("169.254.169.254"); + }); + + it("refuses an empty answer", async () => { + const result = await runLookup(async () => [], true); + expect(result.err).toBeInstanceOf(FetchTargetError); + }); + + it("passes a resolver failure through", async () => { + const failure = Object.assign(new Error("ENOTFOUND"), { + code: "ENOTFOUND", + }); + const result = await runLookup(() => Promise.reject(failure), true); + expect(result.err).toBe(failure); + }); +}); + +function init(overrides: Partial = {}): GuardedFetchInit { + return { + method: "GET", + headers: {}, + body: undefined, + ...overrides, + }; +} + +function redirect(status: number, location: string | null): Response { + return new Response(status === 304 ? null : "moved", { + status, + headers: location === null ? {} : { location }, + }); +} + +/** A hopFetch answering each call from `responses`, in order. */ +function scripted(...responses: Response[]) { + const hopFetch = vi.fn(); + for (const res of responses) hopFetch.mockResolvedValueOnce(res); + return hopFetch; +} + +const resolvesNowhere: ResolveHost = () => + Promise.reject(new Error("ENOTFOUND")); + +describe("guardedFetch hop loop", () => { + it("passes a non-redirect response straight through", async () => { + const hopFetch = scripted(new Response("ok")); + const res = await guardedFetch("http://as.example/x", init(), { + hopFetch, + }); + expect(await res.text()).toBe("ok"); + expect(hopFetch).toHaveBeenCalledWith( + "http://as.example/x", + expect.objectContaining({ redirect: "manual", method: "GET" }), + ); + }); + + it("follows a redirect and re-checks the new hop", async () => { + const hopFetch = scripted( + redirect(302, "http://169.254.169.254/latest/meta-data/"), + ); + await expect( + guardedFetch("http://as.example/x", init(), { hopFetch }), + ).rejects.toThrow(FetchTargetError); + expect(hopFetch).toHaveBeenCalledTimes(1); + }); + + it.each([301, 302, 303, 307, 308])( + "follows a %i to its end", + async (status) => { + const hopFetch = scripted( + redirect(status, "/next"), + new Response("landed"), + ); + const res = await guardedFetch("http://as.example/x", init(), { + hopFetch, + }); + expect(await res.text()).toBe("landed"); + expect(hopFetch.mock.calls[1]?.[0]).toBe("http://as.example/next"); + }, + ); + + // Copilot: only the Fetch-defined redirect statuses are followed. + it.each([300, 304, 305, 306])( + "relays a %i as the response even with a Location", + async (status) => { + const hopFetch = scripted(redirect(status, "/elsewhere")); + const res = await guardedFetch( + "http://as.example/x", + init({ method: "POST", body: "a=b" }), + { hopFetch }, + ); + expect(res.status).toBe(status); + expect(hopFetch).toHaveBeenCalledTimes(1); + }, + ); + + it("returns a redirect status with no Location as the response", async () => { + const hopFetch = scripted(redirect(302, null)); + const res = await guardedFetch("http://as.example/x", init(), { + hopFetch, + }); + expect(res.status).toBe(302); + }); + + it("gives up after the redirect limit", async () => { + const hopFetch = vi + .fn() + .mockImplementation(async () => redirect(302, "/loop")); + await expect( + guardedFetch("http://as.example/x", init(), { hopFetch }), + ).rejects.toThrow(/more than 5 redirects/); + expect(hopFetch).toHaveBeenCalledTimes(MAX_FETCH_REDIRECTS + 1); + }); + + it.each(["file:///etc/passwd", "ftp://as.example/", "data:text/plain,x"])( + "refuses the %s scheme before any request", + async (url) => { + const hopFetch = scripted(); + await expect(guardedFetch(url, init(), { hopFetch })).rejects.toThrow( + /only http and https/, + ); + expect(hopFetch).not.toHaveBeenCalled(); + }, + ); + + it.each([ + "http://169.254.169.254/", + "http://[fe80::1]/", + "http://[::ffff:169.254.169.254]/", + "http://2852039166/", + ])("refuses the IP literal in %s", async (url) => { + const hopFetch = scripted(); + await expect(guardedFetch(url, init(), { hopFetch })).rejects.toThrow( + FetchTargetError, + ); + expect(hopFetch).not.toHaveBeenCalled(); + }); + + it("allows loopback and private literals", async () => { + const hopFetch = scripted(new Response("a"), new Response("b")); + await guardedFetch("http://127.0.0.1:6274/", init(), { hopFetch }); + await guardedFetch("http://192.168.1.10/", init(), { hopFetch }); + expect(hopFetch).toHaveBeenCalledTimes(2); + }); + + it("does not resolve a hostname itself when no proxy is in play", async () => { + const resolveHost = vi.fn(); + const hopFetch = scripted(new Response("ok")); + await guardedFetch("http://as.example/", init(), { + hopFetch, + resolveHost, + viaProxy: false, + }); + expect(resolveHost).not.toHaveBeenCalled(); + }); + + it("forwards method, headers, body and signal on each hop", async () => { + const hopFetch = scripted(new Response("ok")); + const controller = new AbortController(); + await guardedFetch( + "http://as.example/token", + init({ + method: "POST", + headers: { "x-probe": "1" }, + body: "a=b", + signal: controller.signal, + }), + { hopFetch }, + ); + const sent = hopFetch.mock.calls[0]?.[1]; + expect(sent?.method).toBe("POST"); + expect(sent?.body).toBe("a=b"); + expect(sent?.signal).toBe(controller.signal); + expect(new Headers(sent?.headers).get("x-probe")).toBe("1"); + }); +}); + +describe("guardedFetch behind a proxy (best-effort hostname check)", () => { + const viaProxy = true; + + it("refuses a hostname that resolves locally to a blocked address", async () => { + const hopFetch = scripted(); + await expect( + guardedFetch("http://metadata.test/", init(), { + hopFetch, + viaProxy, + resolveHost: async () => ["169.254.169.254"], + }), + ).rejects.toThrow(FetchTargetError); + expect(hopFetch).not.toHaveBeenCalled(); + }); + + it("allows a hostname that resolves locally to a permitted address", async () => { + const hopFetch = scripted(new Response("ok")); + await guardedFetch("http://as.example/", init(), { + hopFetch, + viaProxy, + resolveHost: async () => ["10.1.2.3"], + }); + expect(hopFetch).toHaveBeenCalledTimes(1); + }); + + it("allows a hostname that does not resolve locally", async () => { + const hopFetch = scripted(new Response("ok")); + await guardedFetch("http://as.example/", init(), { + hopFetch, + viaProxy, + resolveHost: resolvesNowhere, + }); + expect(hopFetch).toHaveBeenCalledTimes(1); + }); + + // Copilot: a stalled local resolver must not outlive the caller's abort. + it("rejects promptly when the request aborts mid-lookup", async () => { + const controller = new AbortController(); + const hopFetch = scripted(); + const pending = guardedFetch( + "http://as.example/", + init({ signal: controller.signal }), + { hopFetch, viaProxy, resolveHost: () => new Promise(() => {}) }, + ); + const reason = new Error("deadline"); + controller.abort(reason); + await expect(pending).rejects.toBe(reason); + expect(hopFetch).not.toHaveBeenCalled(); + }); + + it("rejects at once on an already-aborted signal", async () => { + const resolveHost = vi.fn(); + await expect( + guardedFetch( + "http://as.example/", + init({ signal: AbortSignal.abort(new Error("gone")) }), + { hopFetch: scripted(), viaProxy, resolveHost }, + ), + ).rejects.toThrow("gone"); + expect(resolveHost).not.toHaveBeenCalled(); + }); + + it("applies a refusal from a lookup that settles on a live signal", async () => { + const controller = new AbortController(); + await expect( + guardedFetch( + "http://metadata.test/", + init({ signal: controller.signal }), + { + hopFetch: scripted(), + viaProxy, + resolveHost: async () => ["169.254.169.254"], + }, + ), + ).rejects.toThrow(FetchTargetError); + }); + + it("treats a failed lookup on a live signal as unresolvable", async () => { + const controller = new AbortController(); + const hopFetch = scripted(new Response("ok")); + await guardedFetch( + "http://as.example/", + init({ signal: controller.signal }), + { hopFetch, viaProxy, resolveHost: resolvesNowhere }, + ); + expect(hopFetch).toHaveBeenCalledTimes(1); + }); +}); + +describe("createGuardedFetch", () => { + it("passes a string URL, method, headers and string body through", async () => { + const hopFetch = scripted(new Response("ok")); + const guarded = createGuardedFetch({ hopFetch }); + await guarded("http://as.example/register", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: '{"a":1}', + }); + const [url, sent] = hopFetch.mock.calls[0] ?? []; + expect(url).toBe("http://as.example/register"); + expect(sent?.method).toBe("POST"); + expect(sent?.body).toBe('{"a":1}'); + expect(new Headers(sent?.headers).get("content-type")).toBe( + "application/json", + ); + }); + + it("defaults to GET with no body for a URL input", async () => { + const hopFetch = scripted(new Response("ok")); + await createGuardedFetch({ hopFetch })(new URL("http://as.example/m")); + const [url, sent] = hopFetch.mock.calls[0] ?? []; + expect(url).toBe("http://as.example/m"); + expect(sent?.method).toBe("GET"); + expect(sent?.body).toBeUndefined(); + }); + + it("serializes a URLSearchParams form and sets its media type", async () => { + const hopFetch = scripted(new Response("ok")); + await createGuardedFetch({ hopFetch })("http://as.example/token", { + method: "POST", + body: new URLSearchParams({ grant_type: "refresh_token", r: "x y" }), + }); + const sent = hopFetch.mock.calls[0]?.[1]; + expect(sent?.body).toBe("grant_type=refresh_token&r=x+y"); + expect(new Headers(sent?.headers).get("content-type")).toBe( + "application/x-www-form-urlencoded;charset=UTF-8", + ); + }); + + it("keeps a caller's own media type on a URLSearchParams form", async () => { + const hopFetch = scripted(new Response("ok")); + await createGuardedFetch({ hopFetch })("http://as.example/token", { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ a: "b" }), + }); + expect( + new Headers(hopFetch.mock.calls[0]?.[1]?.headers).get("content-type"), + ).toBe("application/x-www-form-urlencoded"); + }); + + it("reads a Request input's method, headers, body and signal", async () => { + const hopFetch = scripted(new Response("ok")); + const controller = new AbortController(); + await createGuardedFetch({ hopFetch })( + new Request("http://as.example/token", { + method: "POST", + headers: { "x-probe": "1" }, + body: "a=b", + signal: controller.signal, + }), + ); + const [url, sent] = hopFetch.mock.calls[0] ?? []; + expect(url).toBe("http://as.example/token"); + expect(sent?.method).toBe("POST"); + expect(sent?.body).toBe("a=b"); + expect(new Headers(sent?.headers).get("x-probe")).toBe("1"); + expect(sent?.signal?.aborted).toBe(false); + }); + + it("reads a bodyless Request input", async () => { + const hopFetch = scripted(new Response("ok")); + await createGuardedFetch({ hopFetch })( + new Request("http://as.example/meta"), + ); + expect(hopFetch.mock.calls[0]?.[1]?.body).toBeUndefined(); + }); + + it("treats a null body as no body", async () => { + const hopFetch = scripted(new Response("ok")); + await createGuardedFetch({ hopFetch })("http://as.example/m", { + body: null, + }); + expect(hopFetch.mock.calls[0]?.[1]?.body).toBeUndefined(); + }); + + it("refuses a body it cannot replay across a redirect", async () => { + await expect( + createGuardedFetch({ hopFetch: scripted() })("http://as.example/m", { + method: "POST", + body: new Uint8Array([1, 2]), + }), + ).rejects.toThrow(/string or URLSearchParams body only/); + }); + + it("rejects with the FetchTargetError itself", async () => { + const refused = new FetchTargetError("refused here"); + const hopFetch = vi + .fn() + .mockRejectedValue(new TypeError("fetch failed", { cause: refused })); + await expect( + createGuardedFetch({ hopFetch })("http://as.example/m"), + ).rejects.toBe(refused); + }); + + it("passes any other failure through unchanged", async () => { + const failure = new TypeError("fetch failed"); + const hopFetch = vi.fn().mockRejectedValue(failure); + await expect( + createGuardedFetch({ hopFetch })("http://as.example/m"), + ).rejects.toBe(failure); + }); +}); diff --git a/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts b/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts deleted file mode 100644 index 1c9e361144..0000000000 --- a/clients/web/src/test/core/mcp/remote/node/fetchGuard.test.ts +++ /dev/null @@ -1,380 +0,0 @@ -/** - * `fetchGuard` — the `/api/fetch` destination guard (#2749). - * - * The block-list, the redirect rewrite and the pinning lookup hook are driven - * directly; the hop loop is driven through an injected `proxyFetch`, which is - * the guard's best-effort (proxied) mode and needs no socket. The direct path, - * which connects through undici, is covered in the integration project - * (`src/test/integration/mcp/remote/fetchGuard.test.ts`). - */ - -import { describe, expect, it, vi } from "vitest"; -import type { LookupAddress } from "node:dns"; -import { - FetchTargetError, - MAX_FETCH_REDIRECTS, - createPinningLookup, - findFetchTargetError, - guardedFetch, - isBlockedAddress, - nextRedirectRequest, - normalizeFetchHeaders, - type FetchHopRequest, - type GuardedFetchInit, - type ResolveHost, -} from "@inspector/core/mcp/remote/node/fetchGuard.js"; - -describe("isBlockedAddress", () => { - it.each([ - "169.254.169.254", - "169.254.0.1", - "::ffff:169.254.169.254", - "::ffff:a9fe:a9fe", - "fe80::1", - "FEBF::1", - "fd00:ec2::254", - ])("blocks %s", (ip) => { - expect(isBlockedAddress(ip)).toBe(true); - }); - - it.each([ - "127.0.0.1", - "10.0.0.1", - "192.168.1.10", - "172.16.0.1", - "169.255.0.1", - "::1", - "::ffff:127.0.0.1", - "::ffff:7f00:1", - "::ffff:not-an-address", - "fec0::1", - "fd00:ec2::253", - "example.com", - ])("allows %s", (ip) => { - expect(isBlockedAddress(ip)).toBe(false); - }); -}); - -describe("normalizeFetchHeaders", () => { - it("keeps string-valued entries only", () => { - expect(normalizeFetchHeaders({ a: "1", b: 2, c: null })).toEqual({ - a: "1", - }); - }); - - it.each([null, "x", ["a"], undefined])("returns {} for %j", (raw) => { - expect(normalizeFetchHeaders(raw)).toEqual({}); - }); -}); - -function hop(overrides: Partial = {}): FetchHopRequest { - return { - url: new URL("https://as.example/token"), - method: "POST", - body: "grant_type=refresh_token", - headers: { - Authorization: "Bearer x", - Cookie: "c=1", - "Content-Type": "application/x-www-form-urlencoded", - Accept: "application/json", - }, - ...overrides, - }; -} - -describe("nextRedirectRequest", () => { - it("rewrites a 302 POST into a bodyless GET and drops body headers", () => { - const next = nextRedirectRequest(hop(), 302, "/elsewhere"); - expect(next.url.href).toBe("https://as.example/elsewhere"); - expect(next.method).toBe("GET"); - expect(next.body).toBeUndefined(); - expect(next.headers).toEqual({ - Authorization: "Bearer x", - Cookie: "c=1", - Accept: "application/json", - }); - }); - - it("keeps a 301 GET as-is", () => { - const next = nextRedirectRequest( - hop({ method: "GET", body: undefined }), - 301, - "/moved", - ); - expect(next.method).toBe("GET"); - expect(next.headers["Content-Type"]).toBe( - "application/x-www-form-urlencoded", - ); - }); - - it("rewrites a 303 PUT but not a 303 HEAD", () => { - expect(nextRedirectRequest(hop({ method: "PUT" }), 303, "/x").method).toBe( - "GET", - ); - expect( - nextRedirectRequest(hop({ method: "HEAD", body: undefined }), 303, "/x") - .method, - ).toBe("HEAD"); - }); - - it("keeps method and body on a same-origin 307", () => { - const next = nextRedirectRequest(hop(), 307, "/again"); - expect(next.method).toBe("POST"); - expect(next.body).toBe("grant_type=refresh_token"); - }); - - it("drops credentials on a cross-origin hop", () => { - const next = nextRedirectRequest( - hop({ method: "GET", body: undefined }), - 302, - "https://other.example/", - ); - expect(next.headers).not.toHaveProperty("Authorization"); - expect(next.headers).not.toHaveProperty("Cookie"); - expect(next.headers.Accept).toBe("application/json"); - }); - - it("refuses a cross-origin hop that would still carry a body", () => { - expect(() => - nextRedirectRequest(hop(), 307, "https://other.example/"), - ).toThrow(FetchTargetError); - }); - - it("does not mutate the current request's headers", () => { - const current = hop(); - nextRedirectRequest(current, 302, "https://other.example/"); - expect(current.headers.Authorization).toBe("Bearer x"); - }); -}); - -describe("findFetchTargetError", () => { - it("finds the error itself or one in its cause chain", () => { - const refused = new FetchTargetError("no"); - expect(findFetchTargetError(refused)).toBe(refused); - expect( - findFetchTargetError(new TypeError("fetch failed", { cause: refused })), - ).toBe(refused); - }); - - it("returns undefined for anything else", () => { - expect(findFetchTargetError(new Error("x"))).toBeUndefined(); - expect(findFetchTargetError("x")).toBeUndefined(); - const cyclic = new Error("a"); - cyclic.cause = cyclic; - expect(findFetchTargetError(cyclic)).toBeUndefined(); - }); -}); - -type LookupResult = { - err: NodeJS.ErrnoException | null; - address: string | LookupAddress[]; - family?: number; -}; - -function runLookup( - resolveHost: ResolveHost, - all: boolean, -): Promise { - const lookup = createPinningLookup(resolveHost); - return new Promise((resolve) => { - lookup("host.test", { all }, (err, address, family) => - resolve({ err, address, family }), - ); - }); -} - -describe("createPinningLookup", () => { - it("hands back every validated address when asked for all", async () => { - const result = await runLookup(async () => ["127.0.0.1", "::1"], true); - expect(result.err).toBeNull(); - expect(result.address).toEqual([ - { address: "127.0.0.1", family: 4 }, - { address: "::1", family: 6 }, - ]); - }); - - it("hands back the first address in the scalar form", async () => { - const result = await runLookup(async () => ["::1", "127.0.0.1"], false); - expect(result).toEqual({ err: null, address: "::1", family: 6 }); - }); - - it("refuses if any resolved address is blocked", async () => { - const result = await runLookup( - async () => ["127.0.0.1", "169.254.169.254"], - true, - ); - expect(result.err).toBeInstanceOf(FetchTargetError); - expect(result.err?.message).toContain("169.254.169.254"); - }); - - it("refuses an empty answer", async () => { - const result = await runLookup(async () => [], true); - expect(result.err).toBeInstanceOf(FetchTargetError); - }); - - it("passes a resolver failure through", async () => { - const failure = Object.assign(new Error("ENOTFOUND"), { - code: "ENOTFOUND", - }); - const result = await runLookup(() => Promise.reject(failure), true); - expect(result.err).toBe(failure); - }); -}); - -function init(overrides: Partial = {}): GuardedFetchInit { - return { - method: "GET", - headers: {}, - body: undefined, - signal: new AbortController().signal, - ...overrides, - }; -} - -function redirect(status: number, location: string | null): Response { - return new Response("moved", { - status, - headers: location === null ? {} : { location }, - }); -} - -/** A proxyFetch answering each call from `responses`, in order. */ -function scripted(...responses: Response[]) { - const proxyFetch = vi.fn(); - for (const res of responses) proxyFetch.mockResolvedValueOnce(res); - return proxyFetch; -} - -const resolvesNowhere: ResolveHost = () => - Promise.reject(new Error("ENOTFOUND")); - -describe("guardedFetch (proxied mode)", () => { - it("passes a non-redirect response straight through", async () => { - const proxyFetch = scripted(new Response("ok")); - const res = await guardedFetch("http://as.example/x", init(), { - proxyFetch, - resolveHost: resolvesNowhere, - }); - expect(await res.text()).toBe("ok"); - expect(proxyFetch).toHaveBeenCalledWith( - "http://as.example/x", - expect.objectContaining({ redirect: "manual", method: "GET" }), - ); - }); - - it("follows a redirect and re-checks the new hop", async () => { - const proxyFetch = scripted( - redirect(302, "http://169.254.169.254/latest/meta-data/"), - ); - await expect( - guardedFetch("http://as.example/x", init(), { - proxyFetch, - resolveHost: resolvesNowhere, - }), - ).rejects.toThrow(FetchTargetError); - expect(proxyFetch).toHaveBeenCalledTimes(1); - }); - - it("follows a permitted redirect to its end", async () => { - const proxyFetch = scripted(redirect(301, "/next"), new Response("landed")); - const res = await guardedFetch("http://as.example/x", init(), { - proxyFetch, - resolveHost: resolvesNowhere, - }); - expect(await res.text()).toBe("landed"); - expect(proxyFetch.mock.calls[1]?.[0]).toBe("http://as.example/next"); - }); - - it("returns a 3xx with no Location as the response", async () => { - const proxyFetch = scripted(redirect(304, null)); - const res = await guardedFetch("http://as.example/x", init(), { - proxyFetch, - resolveHost: resolvesNowhere, - }); - expect(res.status).toBe(304); - }); - - it("gives up after the redirect limit", async () => { - const proxyFetch = vi - .fn() - .mockImplementation(async () => redirect(302, "/loop")); - await expect( - guardedFetch("http://as.example/x", init(), { - proxyFetch, - resolveHost: resolvesNowhere, - }), - ).rejects.toThrow(/more than 5 redirects/); - expect(proxyFetch).toHaveBeenCalledTimes(MAX_FETCH_REDIRECTS + 1); - }); - - it.each(["file:///etc/passwd", "ftp://as.example/", "data:text/plain,x"])( - "refuses the %s scheme before any request", - async (url) => { - const proxyFetch = scripted(); - await expect(guardedFetch(url, init(), { proxyFetch })).rejects.toThrow( - /only http and https/, - ); - expect(proxyFetch).not.toHaveBeenCalled(); - }, - ); - - it.each([ - "http://169.254.169.254/", - "http://[fe80::1]/", - "http://[::ffff:169.254.169.254]/", - "http://2852039166/", - ])("refuses the IP literal in %s", async (url) => { - const proxyFetch = scripted(); - await expect(guardedFetch(url, init(), { proxyFetch })).rejects.toThrow( - FetchTargetError, - ); - expect(proxyFetch).not.toHaveBeenCalled(); - }); - - it("allows loopback and private literals", async () => { - const proxyFetch = scripted(new Response("a"), new Response("b")); - await guardedFetch("http://127.0.0.1:6274/", init(), { proxyFetch }); - await guardedFetch("http://192.168.1.10/", init(), { proxyFetch }); - expect(proxyFetch).toHaveBeenCalledTimes(2); - }); - - it("refuses a hostname that resolves locally to a blocked address", async () => { - const proxyFetch = scripted(); - await expect( - guardedFetch("http://metadata.test/", init(), { - proxyFetch, - resolveHost: async () => ["169.254.169.254"], - }), - ).rejects.toThrow(FetchTargetError); - expect(proxyFetch).not.toHaveBeenCalled(); - }); - - it("allows a hostname that resolves locally to a permitted address", async () => { - const proxyFetch = scripted(new Response("ok")); - await guardedFetch("http://as.example/", init(), { - proxyFetch, - resolveHost: async () => ["10.1.2.3"], - }); - expect(proxyFetch).toHaveBeenCalledTimes(1); - }); - - it("forwards method, headers, body and signal on each hop", async () => { - const proxyFetch = scripted(new Response("ok")); - const controller = new AbortController(); - await guardedFetch( - "http://as.example/token", - init({ - method: "POST", - headers: { "x-probe": "1" }, - body: "a=b", - signal: controller.signal, - }), - { proxyFetch, resolveHost: resolvesNowhere }, - ); - const sent = proxyFetch.mock.calls[0]?.[1]; - expect(sent?.method).toBe("POST"); - expect(sent?.body).toBe("a=b"); - expect(sent?.signal).toBe(controller.signal); - expect(new Headers(sent?.headers).get("x-probe")).toBe("1"); - }); -}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts index bffb999005..95989791bf 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts @@ -12,7 +12,10 @@ import * as os from "node:os"; import * as fs from "node:fs/promises"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; -import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import type { + CreateTransport, + MCPServerConfig, +} from "@inspector/core/mcp/types.js"; import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js"; import { createOAuthClientConfig } from "../helpers/oauth-client-fixtures.js"; import type { InspectorClientOptions } from "@inspector/core/mcp/inspectorClient.js"; @@ -205,4 +208,102 @@ describe("InspectorClient OAuth fetchFn", () => { }), ); }); + + // #2749: `environment.oauthFetch` is the base for OAuth work only. + describe("environment.oauthFetch", () => { + function jsonResponse(): Response { + return new Response("{}", { + headers: { "content-type": "application/json" }, + }); + } + + function newClient( + environment: Partial, + ): InspectorClient { + const oauthConfig = createTestOAuthConfig({ + mode: "static", + clientId: "test-client", + redirectUrl: "http://localhost:3000/callback", + }); + return new InspectorClient( + { type: "sse", url: "http://localhost:3000/sse" } as MCPServerConfig, + { + environment: { + transport: createTransportNode, + oauth: { + storage: oauthConfig.storage, + navigation: oauthConfig.navigation, + redirectUrlProvider: oauthConfig.redirectUrlProvider, + }, + ...environment, + }, + oauth: { clientId: oauthConfig.clientId }, + }, + ); + } + + /** The fetch `InspectorClient` handed to the SDK's `auth()`. */ + function authFetchFromMockAuth(): typeof fetch { + const options = mockAuth.mock.calls[0]?.[1] as + | { fetchFn?: typeof fetch } + | undefined; + if (!options?.fetchFn) throw new Error("auth() got no fetchFn"); + return options.fetchFn; + } + + it("is the auth chain's base when set", async () => { + const plain = vi.fn().mockResolvedValue(jsonResponse()); + const oauthFetch = vi + .fn() + .mockResolvedValue(jsonResponse()); + client = newClient({ fetch: plain, oauthFetch }); + await client.authenticate(); + + await authFetchFromMockAuth()("http://as.example/token"); + expect(oauthFetch).toHaveBeenCalledTimes(1); + expect(plain).not.toHaveBeenCalled(); + }); + + it("leaves the auth chain on environment.fetch when unset", async () => { + const plain = vi.fn().mockResolvedValue(jsonResponse()); + client = newClient({ fetch: plain }); + await client.authenticate(); + + await authFetchFromMockAuth()("http://as.example/token"); + expect(plain).toHaveBeenCalledTimes(1); + }); + + it("takes the transport chain's non-MCP requests, not its MCP ones", async () => { + const plain = vi.fn().mockResolvedValue(jsonResponse()); + const oauthFetch = vi + .fn() + .mockResolvedValue(jsonResponse()); + let transportFetch: typeof fetch | undefined; + // Captures the chain the transport would be built on, then stops the + // connect there; nothing past this point is under test. + const capture: CreateTransport = (_config, options) => { + transportFetch = options.fetchFn; + throw new Error("captured"); + }; + client = newClient({ transport: capture, fetch: plain, oauthFetch }); + await expect(client.connect()).rejects.toThrow(); + if (!transportFetch) throw new Error("no transport fetch captured"); + + // Legacy SSE: the whole configured origin is MCP traffic. + await transportFetch("http://localhost:3000/messages?sessionId=1", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(plain).toHaveBeenCalledTimes(1); + expect(oauthFetch).not.toHaveBeenCalled(); + + // The SDK's own discovery from inside the transport. + await transportFetch( + "http://as.example/.well-known/oauth-authorization-server", + ); + expect(oauthFetch).toHaveBeenCalledTimes(1); + expect(plain).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/node/fetchGuard.test.ts b/clients/web/src/test/integration/mcp/node/fetchGuard.test.ts new file mode 100644 index 0000000000..08bb52c71a --- /dev/null +++ b/clients/web/src/test/integration/mcp/node/fetchGuard.test.ts @@ -0,0 +1,211 @@ +/** + * `guardedFetch` over real sockets — undici with the pinning lookup, direct + * and through `EnvHttpProxyAgent` (#2749). + * + * The hostnames here (`*.test`) exist only in the injected resolvers, so a + * request that reaches the fixture proves the socket connected to the address + * the guard validated rather than to one it resolved for itself. The hop loop, + * the adapter and the pure helpers are covered by the unit suite + * (`src/test/core/mcp/node/fetchGuard.test.ts`). + */ + +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { createServer, type Server } from "node:http"; +import { + FetchTargetError, + findFetchTargetError, + guardedFetch, + type GuardedFetchInit, + type ResolveHost, +} from "@inspector/core/mcp/node/fetchGuard.js"; + +/** Starts an HTTP server on loopback, recording `host path` per request. */ +async function listen( + answer: string, + hits: string[], +): Promise<{ server: Server; port: number }> { + const server = createServer((req, res) => { + hits.push(`${req.headers.host} ${req.url}`); + if (req.url === "/to-metadata-name") { + res.writeHead(302, { Location: `http://metadata.test:${port}/` }); + res.end(); + return; + } + res.writeHead(200, { "Content-Type": "text/plain" }); + res.end(answer); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); + const addr = server.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + return { server, port }; +} + +// The suite owns the proxy environment: an ambient value would route these +// requests elsewhere, and the guard reads it on every call. +const PROXY_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", +] as const; +const savedProxyEnv: Partial> = {}; + +let upstream: { server: Server; port: number }; +const upstreamHits: string[] = []; + +beforeAll(async () => { + for (const name of PROXY_VARS) { + savedProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + upstream = await listen("pinned", upstreamHits); +}); + +afterAll(async () => { + await new Promise((r) => upstream.server.close(() => r())); + for (const name of PROXY_VARS) { + const previous = savedProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } +}); + +/** Resolves `*.test` names from a fixed table. */ +const resolveHost: ResolveHost = async (hostname) => { + if (hostname === "pinned.test") return ["127.0.0.1"]; + if (hostname === "metadata.test") return ["169.254.169.254"]; + throw Object.assign(new Error(`ENOTFOUND ${hostname}`), { + code: "ENOTFOUND", + }); +}; + +function init(): GuardedFetchInit { + return { + method: "GET", + headers: {}, + body: undefined, + signal: AbortSignal.timeout(5_000), + }; +} + +function caught(promise: Promise): Promise { + return promise.then( + () => undefined, + (e: unknown) => e, + ); +} + +describe("guardedFetch (direct)", () => { + it("connects to the address the guard validated", async () => { + const res = await guardedFetch( + `http://pinned.test:${upstream.port}/x`, + init(), + { resolveHost }, + ); + expect(await res.text()).toBe("pinned"); + expect(res).toBeInstanceOf(Response); + expect(upstreamHits).toContain(`pinned.test:${upstream.port} /x`); + }); + + it("refuses a name that resolves to a metadata address", async () => { + const err = await caught( + guardedFetch(`http://metadata.test:${upstream.port}/`, init(), { + resolveHost, + }), + ); + expect(findFetchTargetError(err)).toBeInstanceOf(FetchTargetError); + }); + + it("refuses a redirect to such a name", async () => { + const before = upstreamHits.length; + const err = await caught( + guardedFetch( + `http://pinned.test:${upstream.port}/to-metadata-name`, + init(), + { resolveHost }, + ), + ); + expect(findFetchTargetError(err)?.message).toContain("169.254.169.254"); + expect(upstreamHits.slice(before)).toEqual([ + `pinned.test:${upstream.port} /to-metadata-name`, + ]); + }); + + it("surfaces an unresolvable name as an ordinary fetch failure", async () => { + const err = await caught( + guardedFetch(`http://nowhere.test:${upstream.port}/`, init(), { + resolveHost, + }), + ); + expect(err).toBeInstanceOf(Error); + expect(findFetchTargetError(err)).toBeUndefined(); + }); +}); + +// Copilot (#2796): `EnvHttpProxyAgent` connects a NO_PROXY host directly, so +// that path has to carry the pinning lookup too — the best-effort preflight +// alone would let a rebinding name through. +describe("guardedFetch behind HTTP_PROXY", () => { + let proxy: { server: Server; port: number }; + const proxyHits: string[] = []; + + beforeAll(async () => { + // Never forwards: a request it receives was routed through the proxy, and + // its answer says so. + proxy = await listen("via proxy", proxyHits); + }); + + afterAll(async () => { + await new Promise((r) => proxy.server.close(() => r())); + }); + + afterEach(() => { + delete process.env.HTTP_PROXY; + delete process.env.NO_PROXY; + }); + + it("routes a proxied host through the proxy", async () => { + process.env.HTTP_PROXY = `http://127.0.0.1:${proxy.port}`; + const res = await guardedFetch( + `http://upstream.test:${upstream.port}/p`, + init(), + { resolveHost }, + ); + expect(await res.text()).toBe("via proxy"); + expect(proxyHits).toContain( + `upstream.test:${upstream.port} http://upstream.test:${upstream.port}/p`, + ); + }); + + it("pins a NO_PROXY host's direct connection", async () => { + process.env.HTTP_PROXY = `http://127.0.0.1:${proxy.port}`; + process.env.NO_PROXY = "pinned.test"; + const proxied = proxyHits.length; + const res = await guardedFetch( + `http://pinned.test:${upstream.port}/bypass`, + init(), + // A resolver of its own, so this test builds its own agent. + { resolveHost: async (h) => resolveHost(h) }, + ); + expect(await res.text()).toBe("pinned"); + expect(proxyHits).toHaveLength(proxied); + }); + + it("refuses a NO_PROXY name that rebinds after the preflight", async () => { + process.env.HTTP_PROXY = `http://127.0.0.1:${proxy.port}`; + process.env.NO_PROXY = "rebind.test"; + // Answers the preflight with loopback and the connection with the IMDS. + let calls = 0; + const rebinding: ResolveHost = async () => + calls++ === 0 ? ["127.0.0.1"] : ["169.254.169.254"]; + const err = await caught( + guardedFetch(`http://rebind.test:${upstream.port}/`, init(), { + resolveHost: rebinding, + }), + ); + expect(calls).toBe(2); + expect(findFetchTargetError(err)?.message).toContain("169.254.169.254"); + }); +}); 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 83b8e48a45..a9f6fff776 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -8,7 +8,11 @@ import { vi, } from "vitest"; import { getServerType } from "@inspector/core/mcp/config.js"; -import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { + createBackendBaseFetch, + createTransportNode, +} from "@inspector/core/mcp/node/transport.js"; +import { FetchTargetError } from "@inspector/core/mcp/node/fetchGuard.js"; import { createProxyFetch, readProxyEnv, @@ -790,4 +794,73 @@ describe("Transport", () => { vi.resetModules(); }); }); + + // #2749: the backend's transport refreshes with the tokens the browser + // handed it, so the SDK's OAuth from in here must go through the guard + // while the MCP endpoint itself does not. + describe("createBackendBaseFetch", () => { + // An ambient proxy would replace the plain fetch these tests spy on. + const savedProxyEnv = new Map(); + beforeAll(() => { + for (const name of [ + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", + ]) { + savedProxyEnv.set(name, process.env[name]); + delete process.env[name]; + } + }); + afterAll(() => { + for (const [name, value] of savedProxyEnv) { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; + } + }); + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("sends MCP-endpoint requests to the plain fetch", async () => { + const plain = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("mcp")); + const base = createBackendBaseFetch({ + type: "streamable-http", + url: "http://169.254.10.10/mcp", + }); + // The user configured this server, link-local or not. + const res = await base("http://169.254.10.10/mcp", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(await res.text()).toBe("mcp"); + expect(plain).toHaveBeenCalledTimes(1); + }); + + it("sends OAuth work through the destination guard", async () => { + const plain = vi.spyOn(globalThis, "fetch"); + const base = createBackendBaseFetch({ + type: "sse", + url: "http://127.0.0.1:1/sse", + }); + await expect( + base("http://169.254.169.254/.well-known/oauth-authorization-server"), + ).rejects.toBeInstanceOf(FetchTargetError); + expect(plain).not.toHaveBeenCalled(); + }); + + // A stdio transport makes no HTTP requests at all; with no server URL the + // classifier fails open toward MCP, as it does on every chain it guards. + it("fails open toward the plain fetch with no server URL", async () => { + const plain = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("plain")); + const base = createBackendBaseFetch({ type: "stdio", command: "x" }); + await base("http://as.example/token"); + expect(plain).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts b/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts deleted file mode 100644 index 4e09e3c778..0000000000 --- a/clients/web/src/test/integration/mcp/remote/fetchGuard.test.ts +++ /dev/null @@ -1,104 +0,0 @@ -/** - * `guardedFetch` on its direct path — undici over the pinning lookup (#2749). - * - * The hostnames here (`*.test`) exist only in the injected resolver, so a - * request that reaches the fixture proves the socket connected to the address - * the guard validated rather than to one it resolved for itself. The proxied - * mode and the pure helpers are covered by the unit suite - * (`src/test/core/mcp/remote/node/fetchGuard.test.ts`). - */ - -import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { createServer, type Server } from "node:http"; -import { - FetchTargetError, - findFetchTargetError, - guardedFetch, - type GuardedFetchInit, - type ResolveHost, -} from "@inspector/core/mcp/remote/node/fetchGuard.js"; - -let server: Server; -let port: number; -const hits: string[] = []; - -beforeAll(async () => { - server = createServer((req, res) => { - hits.push(`${req.headers.host} ${req.url}`); - if (req.url === "/to-metadata-name") { - res.writeHead(302, { Location: `http://metadata.test:${port}/` }); - res.end(); - return; - } - res.writeHead(200, { "Content-Type": "text/plain" }); - res.end("pinned"); - }); - await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); - const addr = server.address(); - port = typeof addr === "object" && addr !== null ? addr.port : 0; -}); - -afterAll(async () => { - await new Promise((r) => server.close(() => r())); -}); - -/** Resolves `*.test` names from a fixed table, as a rebinding DNS would. */ -const resolveHost: ResolveHost = async (hostname) => { - if (hostname === "pinned.test") return ["127.0.0.1"]; - if (hostname === "metadata.test") return ["169.254.169.254"]; - throw Object.assign(new Error(`ENOTFOUND ${hostname}`), { - code: "ENOTFOUND", - }); -}; - -function init(): GuardedFetchInit { - return { - method: "GET", - headers: {}, - body: undefined, - signal: AbortSignal.timeout(5_000), - }; -} - -describe("guardedFetch (direct)", () => { - it("connects to the address the guard validated", async () => { - const res = await guardedFetch(`http://pinned.test:${port}/x`, init(), { - resolveHost, - }); - expect(await res.text()).toBe("pinned"); - expect(res).toBeInstanceOf(Response); - expect(hits).toContain(`pinned.test:${port} /x`); - }); - - it("refuses a name that resolves to a metadata address", async () => { - const err: unknown = await guardedFetch( - `http://metadata.test:${port}/`, - init(), - { resolveHost }, - ).catch((e: unknown) => e); - expect(findFetchTargetError(err)).toBeInstanceOf(FetchTargetError); - }); - - it("refuses a redirect to such a name", async () => { - const before = hits.length; - const err: unknown = await guardedFetch( - `http://pinned.test:${port}/to-metadata-name`, - init(), - { resolveHost }, - ).catch((e: unknown) => e); - expect(findFetchTargetError(err)?.message).toContain("169.254.169.254"); - expect(hits.slice(before)).toEqual([ - `pinned.test:${port} /to-metadata-name`, - ]); - }); - - it("surfaces an unresolvable name as an ordinary fetch failure", async () => { - const err: unknown = await guardedFetch( - `http://nowhere.test:${port}/`, - init(), - { resolveHost }, - ).catch((e: unknown) => e); - expect(err).toBeInstanceOf(Error); - expect(findFetchTargetError(err)).toBeUndefined(); - }); -}); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 187ca849e3..6b2c9ef69f 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -332,7 +332,7 @@ export function deadlineForRequestInit( * The request's headers, whatever form `fetch`'s arguments took. `init` wins * over a `Request`'s own, matching `fetch`. */ -function requestHeadersOf( +export function requestHeadersOf( input: RequestInfo | URL, init: RequestInit | undefined, ): Headers { @@ -382,7 +382,7 @@ function cancelBoth( } /** The request URL, whatever form `fetch`'s first argument took. */ -function requestUrlOf(input: RequestInfo | URL): string { +export function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; if (input instanceof URL) return input.href; return input.url; diff --git a/core/auth/routeOAuthRequests.ts b/core/auth/routeOAuthRequests.ts new file mode 100644 index 0000000000..b2d05e6f8b --- /dev/null +++ b/core/auth/routeOAuthRequests.ts @@ -0,0 +1,38 @@ +/** + * Split one fetch chain into MCP traffic and OAuth work (#2749). + * + * A transport's fetch carries both: the MCP endpoint's own requests, and the + * OAuth discovery, token and registration requests the SDK runs from inside + * the transport on a 401 or a refresh. The two need different bases — OAuth + * goes through the destination guard (`core/mcp/node/fetchGuard.ts`), while + * the MCP endpoint is the server the user configured and is never second- + * guessed — so this sends each request to one or the other. + * + * The classification is `exemptMcpEndpoint`'s, the same predicate that keeps + * the MCP endpoint out of the OAuth deadline. It fails open toward MCP when it + * cannot tell, which here means unguarded: the cases it cannot tell are a + * request on the configured server's own origin, which the user chose. + * + * Isomorphic — it only picks between two fetches it is handed — so + * `InspectorClient` can use it in the browser as well as in Node. + */ + +import { requestHeadersOf, requestUrlOf } from "./requestTimeout.js"; + +/** + * A fetch that sends MCP-endpoint requests to `mcpFetch` and everything else + * to `oauthFetch`. + * + * @param isMcpEndpoint - `exemptMcpEndpoint(...)`, or any predicate of the + * same shape; `true` means MCP traffic. + */ +export function routeOAuthRequests( + mcpFetch: typeof fetch, + oauthFetch: typeof fetch, + isMcpEndpoint: (url: string, headers: Headers) => boolean, +): typeof fetch { + return (input, init) => + isMcpEndpoint(requestUrlOf(input), requestHeadersOf(input, init)) + ? mcpFetch(input, init) + : oauthFetch(input, init); +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index a202a748f0..c12f78a18f 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -275,6 +275,7 @@ import { type HandleAuthChallengeOptions, } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; +import { routeOAuthRequests } from "../auth/routeOAuthRequests.js"; import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, @@ -875,10 +876,33 @@ export class InspectorClient extends InspectorClientEventTarget { // `this.fetchFn` directly. The overrides are read lazily — `oauthManager` is // created a few lines below, and `setOAuthConfig` can change them later — so // the wrapper is inert until a server actually configures one. + // + // #2749: OAuth work gets its own base when the environment supplies one — + // the Node clients' destination-guarded fetch. The auth chain sits on it + // outright; the transport chain routes to it everything that is not the + // MCP endpoint (`routeOAuthRequests`), which is exactly the + // transport-internal discovery described above. Absent, both chains share + // the one base as before. + const transportBase = this.fetchFn ?? fetch; + const oauthBase = options.environment.oauthFetch; + // Legacy SSE negotiates its message endpoint inside the stream, so its path + // is unknowable here and the exemption has to widen to the origin. + // Streamable HTTP sends everything to the configured URL, so the path rule + // is exact there and same-origin OAuth stays bounded (and guarded). + const isMcpEndpoint = exemptMcpEndpoint( + () => this.getServerUrl(), + () => this.transportConfig?.type === "sse", + ); + const getOverrides = () => this.oauthManager?.getEndpointOverrides(); const withOverrides = withOAuthEndpointOverrides( - this.fetchFn ?? fetch, - () => this.oauthManager?.getEndpointOverrides(), + oauthBase + ? routeOAuthRequests(transportBase, oauthBase, isMcpEndpoint) + : transportBase, + getOverrides, ); + const authWithOverrides = oauthBase + ? withOAuthEndpointOverrides(oauthBase, getOverrides) + : withOverrides; // #2172: recover discovery when a plain OAuth 2.0 authorization server // publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which // the SDK validates as an OpenID provider document and rejects. Wraps the @@ -900,14 +924,7 @@ export class InspectorClient extends InspectorClientEventTarget { withOAuthRequestTimeout( withOverrides, DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, - exemptMcpEndpoint( - () => this.getServerUrl(), - // Legacy SSE negotiates its message endpoint inside the stream, so - // its path is unknowable here and the exemption has to widen to the - // origin. Streamable HTTP sends everything to the configured URL, so - // the path rule is exact there and same-origin OAuth stays bounded. - () => this.transportConfig?.type === "sse", - ), + isMcpEndpoint, ), ); // #2319: the auth chain is composed separately so the deadline sits @@ -920,7 +937,7 @@ export class InspectorClient extends InspectorClientEventTarget { // chain above: every request on this chain is OAuth work, so there is // nothing here to exempt. this.effectiveAuthFetch = this.buildEffectiveAuthFetch( - withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)), + withRfc8414OidcCompat(withOAuthRequestTimeout(authWithOverrides)), ); this.sessionId = options.sessionId; diff --git a/core/mcp/remote/node/fetchGuard.ts b/core/mcp/node/fetchGuard.ts similarity index 60% rename from core/mcp/remote/node/fetchGuard.ts rename to core/mcp/node/fetchGuard.ts index a43bc98c5a..128b630855 100644 --- a/core/mcp/remote/node/fetchGuard.ts +++ b/core/mcp/node/fetchGuard.ts @@ -1,12 +1,11 @@ /** - * Destination guard for the web backend's `POST /api/fetch` route (#2749). + * Destination guard for the OAuth requests every Node client makes (#2749). * - * The route is the browser's only way out to the network, and it carries the - * web client's OAuth traffic — discovery, protected-resource metadata, token, - * DCR/CIMD, revocation — some of whose URLs come from the server under test. - * Without a guard, a hostile server can point that traffic at the host's - * cloud instance-metadata endpoint and read the answer back through the - * Network log. This is v1's `/fetch` guard (`server/src/proxy-security.ts` + * OAuth discovery, protected-resource metadata, token, DCR/CIMD and revocation + * URLs come partly from the server under test. Without a guard, a hostile + * server can point that traffic at the host's cloud instance-metadata endpoint + * and read the answer back — through the web client's Network log, or a CLI's + * error output. This is v1's `/fetch` guard (`server/src/proxy-security.ts` * and `proxy-redirect.ts` on `v1/main`, #1717 / #1732), ported: * * - **Scheme:** `http:` and `https:` only, on every hop. @@ -18,6 +17,20 @@ * re-validated. The per-hop rewrite the fetch library used to do moves here * with it ({@link nextRedirectRequest}). * + * ## Where it applies + * + * To OAuth traffic only, never to the MCP endpoint a user configured. + * + * - The web backend's `POST /api/fetch` ({@link guardedFetch}), which carries + * every OAuth request the browser makes. + * - `InspectorClientEnvironment.oauthFetch` ({@link createGuardedFetch}), + * which the CLI, TUI and mcpdo fill. `InspectorClient` routes the + * transport chain's non-MCP requests there too (`routeOAuthRequests`), so + * the discovery the SDK runs from inside a transport on a 401 or a refresh + * is covered as well. + * - The web backend's own transport (`createTransportNode`), routed the same + * way, since the SDK refreshes there with the tokens the browser handed it. + * * ## DNS: checked and pinned in one step * * v1 resolved the name, validated the result, and then pinned an agent to it. @@ -30,28 +43,33 @@ * * ## What the guard can promise behind a proxy * - * When `HTTPS_PROXY` / `HTTP_PROXY` is set the route goes through - * `createProxyFetch()`, and the **proxy** resolves the name, not us. Scheme, - * redirect re-validation and IP literals are enforced exactly as above. A - * hostname is resolved locally as a best-effort check — refused if it resolves - * to a blocked address here, allowed if it does not resolve here at all (behind - * a corporate proxy local DNS often cannot see external names) — but it cannot - * be pinned, and the proxy is free to resolve it differently. In that - * configuration the proxy is the network boundary, and keeping the metadata - * service out of reach is its job. + * When `HTTPS_PROXY` / `HTTP_PROXY` is set the request goes through undici's + * `EnvHttpProxyAgent`, built with the same pinning lookup. That agent hands + * `connect` to its `NO_PROXY` agent only — `ProxyAgent` builds its connectors + * from `proxyTls` / `requestTls` — so a host that bypasses the proxy is pinned + * exactly as on the direct path, and the proxy's own address is not checked. * - * ## Out of scope - * - * The CLI and TUI make the same OAuth discovery requests directly from Node, - * without this route. Whether they share the guard is a separate decision. + * A proxied host is resolved by the **proxy**, not by us. Scheme, redirect + * re-validation and IP literals are enforced as above, and the hostname is + * resolved locally as a best-effort check — refused if it resolves to a + * blocked address here, allowed if it does not resolve here at all (behind a + * corporate proxy local DNS often cannot see external names) — but the proxy + * is free to resolve it differently. For that traffic the proxy is the network + * boundary, and keeping the metadata service out of reach is its job. */ import type { LookupAddress } from "node:dns"; import { lookup as dnsLookup } from "node:dns/promises"; import { isIP, type LookupFunction } from "node:net"; -import { asUndiciInit, toGlobalResponse } from "../../node/proxyFetch.js"; - -/** Raised when `/api/fetch` is asked to reach a destination it refuses. */ +import type { Dispatcher } from "undici"; +import { + callerSignalOf, + requestHeadersOf, + requestUrlOf, +} from "../../auth/requestTimeout.js"; +import { asUndiciInit, readProxyEnv, toGlobalResponse } from "./proxyFetch.js"; + +/** Raised when a guarded fetch is asked to reach a destination it refuses. */ export class FetchTargetError extends Error { override name = "FetchTargetError"; } @@ -59,8 +77,17 @@ export class FetchTargetError extends Error { /** Hops followed after the first request before giving up, as in v1. */ export const MAX_FETCH_REDIRECTS = 5; +/** + * The statuses the Fetch standard follows. Any other 3xx — a 300 Multiple + * Choices, a 304 Not Modified — is a response, Location or not, and is + * relayed as one, as the fetch this guard replaced did. + */ +const REDIRECT_STATUSES: ReadonlySet = new Set([ + 301, 302, 303, 307, 308, +]); + /** The one fetch shape this module calls: a string URL and a full init. */ -type HopFetch = (url: string, init: RequestInit) => Promise; +export type HopFetch = (url: string, init: RequestInit) => Promise; /** Resolves a hostname to every address it currently has. */ export type ResolveHost = (hostname: string) => Promise; @@ -73,7 +100,7 @@ const resolveWithSystemDns: ResolveHost = async (hostname) => // --------------------------------------------------------------------------- /** - * Whether `ip` is a link-local or cloud-metadata address that `/api/fetch` + * Whether `ip` is a link-local or cloud-metadata address that a guarded fetch * must never reach. * * - `169.254.0.0/16` (IPv4 link-local; includes the `169.254.169.254` IMDS) @@ -171,29 +198,44 @@ export function createPinningLookup(resolveHost: ResolveHost): LookupFunction { } /** - * One undici `fetch` per resolver, each over a pinned `Agent`, so every - * request with the system resolver shares one connection pool. Keyed weakly so - * a resolver a test injects is collected with it. + * One undici `fetch` per resolver and proxy setting, each over a dispatcher + * built with the pinning lookup, so every request with the system resolver + * shares one connection pool. Keyed weakly on the resolver so one a test + * injects is collected with it; keyed on the proxy URL because + * `EnvHttpProxyAgent` captures it at construction. */ -const directFetches = new WeakMap>(); +const hopFetches = new WeakMap>>(); /** undici is loaded lazily, as `proxyFetch.ts` does, on the first request. */ -async function loadDirectFetch(resolveHost: ResolveHost): Promise { +async function loadHopFetch( + resolveHost: ResolveHost, + proxied: boolean, +): Promise { const undici = await import("undici"); - const dispatcher = new undici.Agent({ - connect: { lookup: createPinningLookup(resolveHost) }, - }); + const connect = { lookup: createPinningLookup(resolveHost) }; + const dispatcher: Dispatcher = proxied + ? new undici.EnvHttpProxyAgent({ connect }) + : new undici.Agent({ connect }); return async (url, init) => toGlobalResponse( await undici.fetch(url, { ...asUndiciInit(init), dispatcher }), ); } -function directFetch(resolveHost: ResolveHost): Promise { - let loaded = directFetches.get(resolveHost); +function hopFetchFor( + resolveHost: ResolveHost, + proxyUrl: string | undefined, +): Promise { + let byProxy = hopFetches.get(resolveHost); + if (byProxy === undefined) { + byProxy = new Map(); + hopFetches.set(resolveHost, byProxy); + } + const key = proxyUrl ?? ""; + let loaded = byProxy.get(key); if (loaded === undefined) { - loaded = loadDirectFetch(resolveHost); - directFetches.set(resolveHost, loaded); + loaded = loadHopFetch(resolveHost, proxyUrl !== undefined); + byProxy.set(key, loaded); } return loaded; } @@ -280,7 +322,7 @@ function rewritesToGet(status: number, method: string): boolean { * {@link BODY_HEADERS} with it. * - **A body never crosses an origin.** A cross-origin hop still carrying a * body after that rewrite — a 307/308 of a POST — is refused, since a - * proxied body here is typically an OAuth form holding a refresh token or + * request body here is typically an OAuth form holding a refresh token or * client secret. * * Returns a fresh headers object every hop, so the caller's input is never @@ -317,34 +359,68 @@ export function nextRedirectRequest( // The guarded fetch // --------------------------------------------------------------------------- -/** The request `/api/fetch` was asked to make. */ +/** The request a guarded fetch was asked to make. */ export interface GuardedFetchInit { method: string; headers: Record; body: string | undefined; - signal: AbortSignal; + signal?: AbortSignal; } export interface GuardedFetchOptions { - /** - * The proxy-aware fetch from `createProxyFetch()`, when a proxy is - * configured. Its presence is what switches the guard to the best-effort - * mode described in the module header. - */ - proxyFetch?: typeof fetch; /** Overrides the system resolver. For tests. */ resolveHost?: ResolveHost; + /** + * Overrides the fetch each hop is made with, which otherwise is undici over + * the pinning lookup. For tests of the hop loop, which then needs no socket. + */ + hopFetch?: HopFetch; + /** + * Whether a proxy is in play, which switches hostnames to the best-effort + * local check described in the module header. Read from the environment + * when absent; set only by tests. + */ + viaProxy?: boolean; +} + +/** + * `resolveHost(host)`, rejecting with the signal's reason as soon as it + * aborts. A local resolver that stalls must not hold a cancelled or timed-out + * request open — the signal otherwise only reaches the fetch after it. + */ +function resolveUnlessAborted( + resolveHost: ResolveHost, + host: string, + signal: AbortSignal | undefined, +): Promise { + if (signal === undefined) return resolveHost(host); + signal.throwIfAborted(); + return new Promise((resolve, reject) => { + const onAbort = () => reject(signal.reason); + signal.addEventListener("abort", onAbort, { once: true }); + resolveHost(host).then( + (addresses) => { + signal.removeEventListener("abort", onAbort); + resolve(addresses); + }, + (err: unknown) => { + signal.removeEventListener("abort", onAbort); + reject(err); + }, + ); + }); } /** * Checks `url` before it is requested. The scheme and an IP literal are * checked on every path; a hostname is checked here only behind a proxy, since - * on the direct path the pinning lookup checks it as the socket connects. + * otherwise the pinning lookup checks it as the socket connects. */ async function assertAllowedTarget( url: URL, resolveHost: ResolveHost, viaProxy: boolean, + signal: AbortSignal | undefined, ): Promise { if (url.protocol !== "http:" && url.protocol !== "https:") { throw new FetchTargetError( @@ -359,8 +435,10 @@ async function assertAllowedTarget( if (!viaProxy) return; let addresses: string[]; try { - addresses = await resolveHost(host); - } catch { + addresses = await resolveUnlessAborted(resolveHost, host, signal); + } catch (err) { + // An abort is the caller giving up, and propagates as one. + if (signal?.aborted) throw err; // Not resolvable here is normal behind a proxy; the proxy resolves it. return; } @@ -368,8 +446,7 @@ async function assertAllowedTarget( } /** - * `fetch(url, init)` for `/api/fetch`, with every hop checked against the - * destination guard. + * `fetch(url, init)` with every hop checked against the destination guard. * * Rejects with a {@link FetchTargetError} — directly, or as the `cause` of the * fetch library's own `TypeError` when the pinning lookup refused the @@ -381,9 +458,10 @@ export async function guardedFetch( options: GuardedFetchOptions = {}, ): Promise { const resolveHost = options.resolveHost ?? resolveWithSystemDns; - const viaProxy = options.proxyFetch !== undefined; + const proxyUrl = readProxyEnv(); + const viaProxy = options.viaProxy ?? proxyUrl !== undefined; const fetchFn: HopFetch = - options.proxyFetch ?? (await directFetch(resolveHost)); + options.hopFetch ?? (await hopFetchFor(resolveHost, proxyUrl)); let request: FetchHopRequest = { url: new URL(url), @@ -392,7 +470,7 @@ export async function guardedFetch( headers: init.headers, }; for (let hop = 0; hop <= MAX_FETCH_REDIRECTS; hop++) { - await assertAllowedTarget(request.url, resolveHost, viaProxy); + await assertAllowedTarget(request.url, resolveHost, viaProxy, init.signal); const res = await fetchFn(request.url.href, { method: request.method, headers: new Headers(request.headers), @@ -401,7 +479,7 @@ export async function guardedFetch( redirect: "manual", }); const location = res.headers.get("location"); - if (res.status < 300 || res.status >= 400 || location === null) { + if (!REDIRECT_STATUSES.has(res.status) || location === null) { return res; } // Nobody reads a redirect's body. Not awaited, for the reason the route @@ -430,3 +508,66 @@ export function findFetchTargetError( } return undefined; } + +/** + * A request body as the guard's string form. OAuth sends a string (JSON for + * DCR) or a `URLSearchParams` form (token, revocation), and both are + * replayable across a redirect hop, which a stream is not. Anything else is + * refused loudly rather than silently mangled. + */ +async function bodyText( + input: RequestInfo | URL, + init: RequestInit | undefined, +): Promise { + const body = + init?.body !== undefined + ? init.body + : input instanceof Request && input.body !== null + ? await input.clone().text() + : undefined; + if (body === undefined || body === null) return undefined; + if (typeof body === "string") return body; + if (body instanceof URLSearchParams) return body.toString(); + throw new TypeError( + "A guarded fetch takes a string or URLSearchParams body only", + ); +} + +/** + * The guard as an ordinary `fetch`, for `InspectorClientEnvironment.oauthFetch` + * and the Node clients' standalone OAuth calls. + * + * It replaces `createProxyFetch()` on those paths rather than wrapping it: it + * reads the same proxy variables and routes through the same `EnvHttpProxyAgent` + * class, built with the pinning lookup (see the module header). + * + * A refusal rejects with the {@link FetchTargetError} itself rather than + * undici's generic `fetch failed`, so a CLI prints why. + */ +export function createGuardedFetch( + options: GuardedFetchOptions = {}, +): typeof fetch { + return async (input, init) => { + const headers = Object.fromEntries(requestHeadersOf(input, init)); + const body = await bodyText(input, init); + // `fetch` sets this itself for a `URLSearchParams` body; a string does not. + if ( + init?.body instanceof URLSearchParams && + !Object.keys(headers).some((h) => h.toLowerCase() === "content-type") + ) { + headers["content-type"] = + "application/x-www-form-urlencoded;charset=UTF-8"; + } + const method = + init?.method ?? (input instanceof Request ? input.method : "GET"); + try { + return await guardedFetch( + requestUrlOf(input), + { method, headers, body, signal: callerSignalOf(input, init) }, + options, + ); + } catch (err) { + throw findFetchTargetError(err) ?? err; + } + }; +} diff --git a/core/mcp/node/index.ts b/core/mcp/node/index.ts index 339cbf6487..05e1bf1bf7 100644 --- a/core/mcp/node/index.ts +++ b/core/mcp/node/index.ts @@ -24,3 +24,4 @@ export { export { rehydrateMcpConfigFromKeychain } from "./server-secrets.js"; export { createTransportNode } from "./transport.js"; export { createProxyFetch, readProxyEnv } from "./proxyFetch.js"; +export { createGuardedFetch, FetchTargetError } from "./fetchGuard.js"; diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index 8429978465..349a3574b9 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -17,6 +17,9 @@ import { createAuthChallengeObserverFetch, } from "./authChallengeFetch.js"; import { createProxyFetch } from "./proxyFetch.js"; +import { createGuardedFetch } from "./fetchGuard.js"; +import { exemptMcpEndpoint } from "../../auth/requestTimeout.js"; +import { routeOAuthRequests } from "../../auth/routeOAuthRequests.js"; import { createNotificationHeadersFetch } from "./notificationHeadersFetch.js"; import { createSuppressNotificationStreamFetch } from "./suppressNotificationStreamFetch.js"; @@ -37,6 +40,26 @@ function headersFromSettings( return Object.keys(out).length > 0 ? out : undefined; } +/** + * The base fetch for a transport whose caller supplies none — the web backend. + * + * MCP-endpoint requests go out on the proxy-aware plain fetch. Everything else + * on the chain is the OAuth work the SDK runs from inside the transport on a + * 401 or a refresh, with the tokens the browser handed the backend and on URLs + * the server under test chose, so it goes through the destination guard + * (#2749), as the browser's own OAuth does via `/api/fetch`. + */ +export function createBackendBaseFetch(config: MCPServerConfig): typeof fetch { + return routeOAuthRequests( + createProxyFetch() ?? globalThis.fetch, + createGuardedFetch(), + exemptMcpEndpoint( + () => ("url" in config ? config.url : undefined), + () => getServerType(config) === "sse", + ), + ); +} + /** * Creates the appropriate transport for an MCP server configuration. */ @@ -68,8 +91,9 @@ export function createTransportNode( // a proxy-aware base when one is needed — the Node clients install // `createProxyFetch()` as `environment.fetch`, and `InspectorClient` wraps // that. The fallback is for the one caller that supplies nothing: the web - // backend (`core/mcp/remote/node/server.ts`), which calls this directly. - const baseFetch = optionsFetchFn ?? createProxyFetch() ?? globalThis.fetch; + // backend (`core/mcp/remote/node/server.ts`), which calls this directly, + // and it routes OAuth through the destination guard (#2749). + const baseFetch = optionsFetchFn ?? createBackendBaseFetch(config); // Purely passive, so — unlike proxying and interception — it is safe to // apply to any fetch, including a caller's explicit one. const withChallengeObserver = (inner: typeof fetch): typeof fetch => diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 4b7ca51f4b..db64eb74c9 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -27,13 +27,12 @@ import { streamSSE } from "hono/streaming"; import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; -import { createProxyFetch } from "../../node/proxyFetch.js"; import { declaresOversizeBody, readCappedText } from "./cappedBody.js"; import { findFetchTargetError, guardedFetch, normalizeFetchHeaders, -} from "./fetchGuard.js"; +} from "../../node/fetchGuard.js"; import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; import { redactUrlQuery } from "../../fetchTracking.js"; import { isRateLimitError } from "../../rateLimit.js"; @@ -1395,23 +1394,20 @@ export function createRemoteApp( // requests here. Left on the global `fetch`, a corporate-proxy user could // connect to a server but never authorize against it — and Node's native // `NODE_USE_ENV_PROXY` does not cover them either, being unsupported at - // our 22.19 engine floor (#2067). + // our 22.19 engine floor (#2067). The guard (#2749) reads the same proxy + // variables and routes through the same `EnvHttpProxyAgent` class. // - // And guarded (#2749): some of these URLs come from the server under - // test, so every hop is checked against the link-local / metadata - // block-list, redirects are followed there rather than by the fetch, - // and the direct path pins each connection to the addresses it checked. - // What the guard can and cannot promise behind a proxy is its header. - const res = await guardedFetch( - url, - { - method, - headers: normalizeFetchHeaders(headers), - body: reqBody, - signal, - }, - { proxyFetch: createProxyFetch() }, - ); + // Guarded because some of these URLs come from the server under test: + // every hop is checked against the link-local / metadata block-list, + // redirects are followed there rather than by the fetch, and each + // connection the guard opens is pinned to the addresses it checked. + // What it can and cannot promise behind a proxy is its header. + const res = await guardedFetch(url, { + method, + headers: normalizeFetchHeaders(headers), + body: reqBody, + signal, + }); const resHeaders: Record = {}; res.headers.forEach((v, k) => { diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 7cde329006..513dbd12e6 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -1235,6 +1235,20 @@ export interface InspectorClientEnvironment { */ fetch?: typeof fetch; + /** + * Optional base fetch for OAuth work only — discovery, protected-resource + * metadata, DCR/CIMD, token and revocation — never the MCP endpoint (#2749). + * When set, `InspectorClient` builds its auth chain on it and routes the + * transport chain's non-MCP requests (the SDK's own discovery on a 401 or a + * refresh) to it as well; absent, OAuth shares `fetch`. + * - Node: `createGuardedFetch()` (core/mcp/node/fetchGuard.ts) — refuses + * link-local and cloud-metadata destinations, proxy-aware like + * `createProxyFetch()`. + * - Browser: unset; the browser's OAuth already goes through the guarded + * `/api/fetch` route via `createRemoteFetch`. + */ + oauthFetch?: typeof fetch; + /** * Optional logger for InspectorClient events (transport, OAuth, etc.). * - Node: pino file logger From c08a719946bca77d1b21b37a19d1f4fd4f930ae4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 24 Oct 2026 23:50:44 -0400 Subject: [PATCH 3/4] fix: route well-known URIs to the OAuth guard under legacy SSE (#2749) Legacy SSE widens exemptMcpEndpoint's MCP exemption to the server's whole origin, so a hostile SSE server's own /.well-known/oauth-protected-resource went out on the unguarded fetch and could redirect discovery to the metadata service (Copilot, round 2). An RFC 8615 well-known URI is never MCP traffic, so routeOAuthRequests now sends any path containing /.well-known/ to the guard ahead of the classifier. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../test/core/auth/routeOAuthRequests.test.ts | 45 +++++++++++++++++-- .../mcp/inspectorClient-oauth-fetchFn.test.ts | 8 ++++ .../integration/mcp/node/transport.test.ts | 28 ++++++++++++ core/auth/routeOAuthRequests.ts | 32 ++++++++++--- 4 files changed, 104 insertions(+), 9 deletions(-) diff --git a/clients/web/src/test/core/auth/routeOAuthRequests.test.ts b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts index 6a033da04a..587457f2d2 100644 --- a/clients/web/src/test/core/auth/routeOAuthRequests.test.ts +++ b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts @@ -4,7 +4,10 @@ */ import { describe, expect, it, vi } from "vitest"; -import { routeOAuthRequests } from "@inspector/core/auth/routeOAuthRequests.js"; +import { + isWellKnownUri, + routeOAuthRequests, +} from "@inspector/core/auth/routeOAuthRequests.js"; import { exemptMcpEndpoint } from "@inspector/core/auth/requestTimeout.js"; function fetches() { @@ -25,9 +28,7 @@ describe("routeOAuthRequests", () => { ); expect(mcp).toHaveBeenCalledWith("http://s.example/mcp", mcpInit); - const url = new URL( - "http://as.example/.well-known/oauth-protected-resource", - ); + const url = new URL("http://as.example/token"); expect(await (await routed(url)).text()).toBe("oauth"); expect(oauth).toHaveBeenCalledWith(url, undefined); expect(isMcp).toHaveBeenLastCalledWith(url.href, expect.any(Headers)); @@ -68,4 +69,40 @@ describe("routeOAuthRequests", () => { expect(mcp).toHaveBeenCalledTimes(1); expect(oauth).toHaveBeenCalledTimes(2); }); + + // Copilot (#2796): legacy SSE widens the MCP exemption to the whole origin, + // which must not swallow the server's own discovery documents. + it("sends a same-origin well-known URI to OAuth under legacy SSE", async () => { + const { mcp, oauth } = fetches(); + const routed = routeOAuthRequests( + mcp, + oauth, + exemptMcpEndpoint( + () => "http://s.example/sse", + () => true, + ), + ); + await routed("http://s.example/messages?sessionId=1", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + await routed("http://s.example/.well-known/oauth-protected-resource"); + await routed("http://s.example/tenant/.well-known/openid-configuration"); + expect(mcp).toHaveBeenCalledTimes(1); + expect(oauth).toHaveBeenCalledTimes(2); + }); +}); + +describe("isWellKnownUri", () => { + it.each([ + ["http://s.example/.well-known/oauth-authorization-server", true], + ["http://s.example/.well-known/oauth-protected-resource/mcp", true], + ["http://s.example/tenant/.well-known/openid-configuration", true], + ["http://s.example/mcp", false], + ["http://s.example/well-known/x", false], + ["not a url", false], + ])("%s → %s", (url, expected) => { + expect(isWellKnownUri(url)).toBe(expected); + }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts index 95989791bf..d7e4e89a4a 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts @@ -304,6 +304,14 @@ describe("InspectorClient OAuth fetchFn", () => { ); expect(oauthFetch).toHaveBeenCalledTimes(1); expect(plain).toHaveBeenCalledTimes(1); + + // Same-origin discovery is OAuth too, despite the origin-wide exemption + // (Copilot, #2796). + await transportFetch( + "http://localhost:3000/.well-known/oauth-protected-resource", + ); + expect(oauthFetch).toHaveBeenCalledTimes(2); + expect(plain).toHaveBeenCalledTimes(1); }); }); }); 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 a9f6fff776..49bad99c61 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -852,6 +852,34 @@ describe("Transport", () => { expect(plain).not.toHaveBeenCalled(); }); + // Copilot (#2796): legacy SSE's origin-wide MCP exemption must not carry + // the server's own discovery document past the guard. + it("guards a same-origin discovery redirect under legacy SSE", async () => { + const hostile = createServer((req, res) => { + if (req.url === "/.well-known/oauth-protected-resource") { + res.writeHead(302, { Location: "http://169.254.169.254/latest/" }); + } else { + res.writeHead(404); + } + res.end(); + }); + await new Promise((r) => hostile.listen(0, "127.0.0.1", () => r())); + const { port } = hostile.address() as AddressInfo; + const plain = vi.spyOn(globalThis, "fetch"); + try { + const base = createBackendBaseFetch({ + type: "sse", + url: `http://127.0.0.1:${port}/sse`, + }); + await expect( + base(`http://127.0.0.1:${port}/.well-known/oauth-protected-resource`), + ).rejects.toBeInstanceOf(FetchTargetError); + expect(plain).not.toHaveBeenCalled(); + } finally { + await new Promise((r) => hostile.close(() => r())); + } + }); + // A stdio transport makes no HTTP requests at all; with no server URL the // classifier fails open toward MCP, as it does on every chain it guards. it("fails open toward the plain fetch with no server URL", async () => { diff --git a/core/auth/routeOAuthRequests.ts b/core/auth/routeOAuthRequests.ts index b2d05e6f8b..e8a74bf07b 100644 --- a/core/auth/routeOAuthRequests.ts +++ b/core/auth/routeOAuthRequests.ts @@ -9,9 +9,14 @@ * guessed — so this sends each request to one or the other. * * The classification is `exemptMcpEndpoint`'s, the same predicate that keeps - * the MCP endpoint out of the OAuth deadline. It fails open toward MCP when it - * cannot tell, which here means unguarded: the cases it cannot tell are a - * request on the configured server's own origin, which the user chose. + * the MCP endpoint out of the OAuth deadline, with one override ahead of it: + * a well-known URI ({@link isWellKnownUri}) is always OAuth. Legacy SSE widens + * that predicate's exemption to the server's whole origin, so without the + * override a hostile SSE server's own `/.well-known/oauth-protected-resource` + * would go out unguarded and could redirect discovery to the metadata service + * (Copilot). Otherwise the predicate fails open toward MCP when it cannot + * tell, which here means unguarded: those cases are requests on the configured + * server's own origin, which the user chose. * * Isomorphic — it only picks between two fetches it is handed — so * `InspectorClient` can use it in the browser as well as in Node. @@ -19,6 +24,20 @@ import { requestHeadersOf, requestUrlOf } from "./requestTimeout.js"; +/** + * Whether `url` is an RFC 8615 well-known URI — OAuth and OIDC discovery, + * never MCP traffic. Matched anywhere in the path, not only as a prefix, for + * the OIDC discovery form that appends it to an issuer's path + * (`/tenant/.well-known/openid-configuration`). + */ +export function isWellKnownUri(url: string): boolean { + try { + return new URL(url).pathname.includes("/.well-known/"); + } catch { + return false; + } +} + /** * A fetch that sends MCP-endpoint requests to `mcpFetch` and everything else * to `oauthFetch`. @@ -31,8 +50,11 @@ export function routeOAuthRequests( oauthFetch: typeof fetch, isMcpEndpoint: (url: string, headers: Headers) => boolean, ): typeof fetch { - return (input, init) => - isMcpEndpoint(requestUrlOf(input), requestHeadersOf(input, init)) + return (input, init) => { + const url = requestUrlOf(input); + return !isWellKnownUri(url) && + isMcpEndpoint(url, requestHeadersOf(input, init)) ? mcpFetch(input, init) : oauthFetch(input, init); + }; } From 903c04c03bbf3baae54a5a20dcf2d42bec9c1756 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 25 Oct 2026 00:13:52 -0400 Subject: [PATCH 4/4] fix: identify MCP traffic by request shape, not origin alone (#2749, #2802) Under legacy SSE, exemptMcpEndpoint widens its MCP exemption to the server's whole origin, so any same-origin OAuth request that was neither form-encoded nor at a well-known path - protected-resource metadata at a custom resource_metadata path (Copilot, round 3), or dynamic client registration (#2802) - still went out unguarded and could be redirected to the metadata service. routeOAuthRequests now requires a request to have an MCP wire shape as well as an MCP destination: a JSON-RPC 2.0 POST (message or batch), an event-stream GET, or a session DELETE. OAuth sends none of those, and anything else goes to the guard, which only refuses link-local and metadata addresses. This supersedes the well-known-URI override from round 2. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../test/core/auth/routeOAuthRequests.test.ts | 143 +++++++++++------- .../mcp/inspectorClient-oauth-fetchFn.test.ts | 11 +- .../integration/mcp/node/transport.test.ts | 19 ++- core/auth/routeOAuthRequests.ts | 98 +++++++++--- 4 files changed, 183 insertions(+), 88 deletions(-) diff --git a/clients/web/src/test/core/auth/routeOAuthRequests.test.ts b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts index 587457f2d2..7d78487f70 100644 --- a/clients/web/src/test/core/auth/routeOAuthRequests.test.ts +++ b/clients/web/src/test/core/auth/routeOAuthRequests.test.ts @@ -5,7 +5,7 @@ import { describe, expect, it, vi } from "vitest"; import { - isWellKnownUri, + isMcpShapedRequest, routeOAuthRequests, } from "@inspector/core/auth/routeOAuthRequests.js"; import { exemptMcpEndpoint } from "@inspector/core/auth/requestTimeout.js"; @@ -16,36 +16,87 @@ function fetches() { return { mcp, oauth }; } +const RPC = JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }); + +/** An MCP message POST, as every transport sends one. */ +function rpcPost(body: string = RPC): RequestInit { + return { + method: "POST", + headers: { "content-type": "application/json" }, + body, + }; +} + +describe("isMcpShapedRequest", () => { + it.each<[string, RequestInfo | URL, RequestInit | undefined, boolean]>([ + ["a JSON-RPC POST", "http://s/mcp", rpcPost(), true], + ["a JSON-RPC batch POST", "http://s/mcp", rpcPost(`[${RPC},${RPC}]`), true], + ["an empty batch", "http://s/mcp", rpcPost("[]"), false], + ["a mixed batch", "http://s/mcp", rpcPost(`[${RPC},{"a":1}]`), false], + ["a non-JSON-RPC JSON POST (DCR)", "http://s/r", rpcPost('{"a":1}'), false], + ["a JSON null POST", "http://s/r", rpcPost("null"), false], + ["a non-JSON POST", "http://s/r", rpcPost("grant_type=x"), false], + [ + "a URLSearchParams POST", + "http://s/t", + { method: "post", body: new URLSearchParams({ a: "b" }) }, + false, + ], + [ + "an event-stream GET", + "http://s/sse", + { headers: { Accept: "Text/Event-Stream" } }, + true, + ], + ["a plain GET (discovery)", "http://s/meta", undefined, false], + ["a DELETE", "http://s/mcp", { method: "DELETE" }, true], + ["a PUT", "http://s/mcp", { method: "PUT", body: RPC }, false], + [ + "a Request GET accepting event-stream", + new Request("http://s/sse", { headers: { accept: "text/event-stream" } }), + undefined, + true, + ], + [ + "a Request POST (body unreadable synchronously)", + new Request("http://s/mcp", { method: "POST", body: RPC }), + undefined, + false, + ], + ])("%s → %s", (_label, input, init, expected) => { + expect(isMcpShapedRequest(input, init)).toBe(expected); + }); +}); + describe("routeOAuthRequests", () => { - it("routes by the predicate, handing each fetch the original arguments", async () => { + it("sends MCP-shaped traffic to the endpoint to mcpFetch, with the original arguments", async () => { const { mcp, oauth } = fetches(); - const isMcp = vi.fn((url: string) => url.endsWith("/mcp")); + const isMcp = vi.fn<(url: string, headers: Headers) => boolean>(() => true); const routed = routeOAuthRequests(mcp, oauth, isMcp); - - const mcpInit = { method: "POST", body: "{}" }; - expect(await (await routed("http://s.example/mcp", mcpInit)).text()).toBe( + const init = rpcPost(); + expect(await (await routed("http://s.example/mcp", init)).text()).toBe( "mcp", ); - expect(mcp).toHaveBeenCalledWith("http://s.example/mcp", mcpInit); + expect(mcp).toHaveBeenCalledWith("http://s.example/mcp", init); + const [seenUrl, seenHeaders] = isMcp.mock.calls[0] ?? []; + expect(seenUrl).toBe("http://s.example/mcp"); + expect(seenHeaders?.get("content-type")).toBe("application/json"); + }); - const url = new URL("http://as.example/token"); - expect(await (await routed(url)).text()).toBe("oauth"); - expect(oauth).toHaveBeenCalledWith(url, undefined); - expect(isMcp).toHaveBeenLastCalledWith(url.href, expect.any(Headers)); + it("sends MCP-shaped traffic elsewhere to oauthFetch", async () => { + const { mcp, oauth } = fetches(); + const url = new URL("http://other.example/mcp"); + await routeOAuthRequests(mcp, oauth, () => false)(url, rpcPost()); + expect(oauth).toHaveBeenCalledWith(url, rpcPost()); + expect(mcp).not.toHaveBeenCalled(); }); - it("hands the predicate a Request's URL and headers", async () => { + it("sends a non-MCP shape to oauthFetch without consulting the predicate", async () => { const { mcp, oauth } = fetches(); - const isMcp = vi.fn<(url: string, headers: Headers) => boolean>( - () => false, - ); - const req = new Request("http://as.example/token", { - headers: { "x-probe": "1" }, - }); - await routeOAuthRequests(mcp, oauth, isMcp)(req); - const [seenUrl, seenHeaders] = isMcp.mock.calls[0] ?? []; - expect(seenUrl).toBe("http://as.example/token"); - expect(seenHeaders?.get("x-probe")).toBe("1"); + const isMcp = vi.fn(() => true); + await routeOAuthRequests(mcp, oauth, isMcp)("http://s.example/meta"); + expect(oauth).toHaveBeenCalledTimes(1); + expect(isMcp).not.toHaveBeenCalled(); }); it("sends a same-path OAuth form to OAuth under exemptMcpEndpoint", async () => { @@ -55,24 +106,20 @@ describe("routeOAuthRequests", () => { oauth, exemptMcpEndpoint(() => "http://s.example/mcp"), ); - await routed("http://s.example/mcp", { - method: "POST", - headers: { "content-type": "application/json" }, - body: "{}", - }); + await routed("http://s.example/mcp", rpcPost()); await routed("http://s.example/mcp", { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, body: "grant_type=refresh_token", }); - await routed("http://as.example/token"); expect(mcp).toHaveBeenCalledTimes(1); - expect(oauth).toHaveBeenCalledTimes(2); + expect(oauth).toHaveBeenCalledTimes(1); }); - // Copilot (#2796): legacy SSE widens the MCP exemption to the whole origin, - // which must not swallow the server's own discovery documents. - it("sends a same-origin well-known URI to OAuth under legacy SSE", async () => { + // Copilot (#2796, rounds 2 and 3): legacy SSE widens the MCP exemption to + // the whole origin, which must not swallow the server's own OAuth requests — + // discovery at a well-known or a custom path, or a registration POST. + it("keeps same-origin OAuth on the guard under legacy SSE", async () => { const { mcp, oauth } = fetches(); const routed = routeOAuthRequests( mcp, @@ -82,27 +129,19 @@ describe("routeOAuthRequests", () => { () => true, ), ); - await routed("http://s.example/messages?sessionId=1", { - method: "POST", - headers: { "content-type": "application/json" }, - body: "{}", + await routed("http://s.example/sse", { + headers: { accept: "text/event-stream" }, }); - await routed("http://s.example/.well-known/oauth-protected-resource"); - await routed("http://s.example/tenant/.well-known/openid-configuration"); - expect(mcp).toHaveBeenCalledTimes(1); - expect(oauth).toHaveBeenCalledTimes(2); - }); -}); + await routed("http://s.example/messages?sessionId=1", rpcPost()); + expect(mcp).toHaveBeenCalledTimes(2); -describe("isWellKnownUri", () => { - it.each([ - ["http://s.example/.well-known/oauth-authorization-server", true], - ["http://s.example/.well-known/oauth-protected-resource/mcp", true], - ["http://s.example/tenant/.well-known/openid-configuration", true], - ["http://s.example/mcp", false], - ["http://s.example/well-known/x", false], - ["not a url", false], - ])("%s → %s", (url, expected) => { - expect(isWellKnownUri(url)).toBe(expected); + await routed("http://s.example/.well-known/oauth-protected-resource"); + await routed("http://s.example/custom/protected-resource"); + await routed( + "http://s.example/register", + rpcPost('{"redirect_uris":["http://localhost/cb"]}'), + ); + expect(oauth).toHaveBeenCalledTimes(3); + expect(mcp).toHaveBeenCalledTimes(2); }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts index d7e4e89a4a..ea9588547b 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-fetchFn.test.ts @@ -289,11 +289,11 @@ describe("InspectorClient OAuth fetchFn", () => { await expect(client.connect()).rejects.toThrow(); if (!transportFetch) throw new Error("no transport fetch captured"); - // Legacy SSE: the whole configured origin is MCP traffic. + // Legacy SSE: a JSON-RPC message anywhere on the origin is MCP traffic. await transportFetch("http://localhost:3000/messages?sessionId=1", { method: "POST", headers: { "content-type": "application/json" }, - body: "{}", + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "ping" }), }); expect(plain).toHaveBeenCalledTimes(1); expect(oauthFetch).not.toHaveBeenCalled(); @@ -305,12 +305,13 @@ describe("InspectorClient OAuth fetchFn", () => { expect(oauthFetch).toHaveBeenCalledTimes(1); expect(plain).toHaveBeenCalledTimes(1); - // Same-origin discovery is OAuth too, despite the origin-wide exemption - // (Copilot, #2796). + // Same-origin discovery is OAuth too, at a well-known or a custom path, + // despite the origin-wide exemption (Copilot, #2796). await transportFetch( "http://localhost:3000/.well-known/oauth-protected-resource", ); - expect(oauthFetch).toHaveBeenCalledTimes(2); + await transportFetch("http://localhost:3000/custom/protected-resource"); + expect(oauthFetch).toHaveBeenCalledTimes(3); expect(plain).toHaveBeenCalledTimes(1); }); }); 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 49bad99c61..461129e30f 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -834,7 +834,7 @@ describe("Transport", () => { const res = await base("http://169.254.10.10/mcp", { method: "POST", headers: { "content-type": "application/json" }, - body: "{}", + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "ping" }), }); expect(await res.text()).toBe("mcp"); expect(plain).toHaveBeenCalledTimes(1); @@ -853,10 +853,14 @@ describe("Transport", () => { }); // Copilot (#2796): legacy SSE's origin-wide MCP exemption must not carry - // the server's own discovery document past the guard. - it("guards a same-origin discovery redirect under legacy SSE", async () => { + // the server's own discovery documents past the guard, at a well-known + // path or a custom `resource_metadata` one. + it.each([ + "/.well-known/oauth-protected-resource", + "/custom/protected-resource", + ])("guards a same-origin %s redirect under legacy SSE", async (path) => { const hostile = createServer((req, res) => { - if (req.url === "/.well-known/oauth-protected-resource") { + if (req.url === path) { res.writeHead(302, { Location: "http://169.254.169.254/latest/" }); } else { res.writeHead(404); @@ -872,7 +876,7 @@ describe("Transport", () => { url: `http://127.0.0.1:${port}/sse`, }); await expect( - base(`http://127.0.0.1:${port}/.well-known/oauth-protected-resource`), + base(`http://127.0.0.1:${port}${path}`), ).rejects.toBeInstanceOf(FetchTargetError); expect(plain).not.toHaveBeenCalled(); } finally { @@ -887,7 +891,10 @@ describe("Transport", () => { .spyOn(globalThis, "fetch") .mockResolvedValue(new Response("plain")); const base = createBackendBaseFetch({ type: "stdio", command: "x" }); - await base("http://as.example/token"); + await base("http://as.example/mcp", { + method: "POST", + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "ping" }), + }); expect(plain).toHaveBeenCalledTimes(1); }); }); diff --git a/core/auth/routeOAuthRequests.ts b/core/auth/routeOAuthRequests.ts index e8a74bf07b..619e0accb9 100644 --- a/core/auth/routeOAuthRequests.ts +++ b/core/auth/routeOAuthRequests.ts @@ -8,15 +8,25 @@ * the MCP endpoint is the server the user configured and is never second- * guessed — so this sends each request to one or the other. * - * The classification is `exemptMcpEndpoint`'s, the same predicate that keeps - * the MCP endpoint out of the OAuth deadline, with one override ahead of it: - * a well-known URI ({@link isWellKnownUri}) is always OAuth. Legacy SSE widens - * that predicate's exemption to the server's whole origin, so without the - * override a hostile SSE server's own `/.well-known/oauth-protected-resource` - * would go out unguarded and could redirect discovery to the metadata service - * (Copilot). Otherwise the predicate fails open toward MCP when it cannot - * tell, which here means unguarded: those cases are requests on the configured - * server's own origin, which the user chose. + * A request is MCP traffic only when it passes **both** of two tests: + * + * - **Where it goes** — `exemptMcpEndpoint`'s classification, the same + * predicate that keeps the MCP endpoint out of the OAuth deadline. + * - **What it looks like** ({@link isMcpShapedRequest}) — a JSON-RPC POST, an + * event-stream GET, or a session DELETE. + * + * The second test exists because the first cannot be exact for legacy SSE. + * Its message endpoint is negotiated inside the stream, so the predicate + * widens to the server's whole origin, and every same-origin OAuth request — + * protected-resource metadata at a custom `resource_metadata` path, discovery + * at a well-known one, dynamic client registration — would otherwise go out + * unguarded and could be redirected to the metadata service (Copilot). OAuth + * never sends any of the three MCP shapes, so the shape test settles what the + * origin cannot. + * + * Anything else goes to the guard. That is the cheap direction to be wrong in: + * the guard refuses only link-local and metadata addresses, so an MCP request + * it receives by mistake still reaches an ordinary server. * * Isomorphic — it only picks between two fetches it is handed — so * `InspectorClient` can use it in the browser as well as in Node. @@ -24,37 +34,75 @@ import { requestHeadersOf, requestUrlOf } from "./requestTimeout.js"; -/** - * Whether `url` is an RFC 8615 well-known URI — OAuth and OIDC discovery, - * never MCP traffic. Matched anywhere in the path, not only as a prefix, for - * the OIDC discovery form that appends it to an issuer's path - * (`/tenant/.well-known/openid-configuration`). - */ -export function isWellKnownUri(url: string): boolean { +/** Whether `value` is one JSON-RPC 2.0 message. */ +function isJsonRpcMessage(value: unknown): boolean { + return ( + typeof value === "object" && + value !== null && + (value as { jsonrpc?: unknown }).jsonrpc === "2.0" + ); +} + +/** Whether `body` is a JSON-RPC 2.0 message or a non-empty batch of them. */ +function isJsonRpcBody(body: unknown): boolean { + if (typeof body !== "string") return false; + let parsed: unknown; try { - return new URL(url).pathname.includes("/.well-known/"); + parsed = JSON.parse(body); } catch { return false; } + return Array.isArray(parsed) + ? parsed.length > 0 && parsed.every(isJsonRpcMessage) + : isJsonRpcMessage(parsed); +} + +/** + * Whether a request has one of the shapes MCP traffic takes on the wire, none + * of which OAuth ever sends: + * + * - a POST whose body is a JSON-RPC 2.0 message or batch (every transport's + * client-to-server messages); + * - a GET that accepts `text/event-stream` (the legacy SSE stream, and + * streamable HTTP's standalone stream); + * - a DELETE (streamable HTTP's session termination). + * + * The body is read only when it is a string, which is how the SDK sends every + * message; a `Request` input's body cannot be read synchronously and so does + * not qualify, which routes it to the guard. + */ +export function isMcpShapedRequest( + input: RequestInfo | URL, + init: RequestInit | undefined, +): boolean { + const method = ( + init?.method ?? (input instanceof Request ? input.method : "GET") + ).toUpperCase(); + if (method === "DELETE") return true; + if (method === "POST") return isJsonRpcBody(init?.body); + if (method === "GET") { + return (requestHeadersOf(input, init).get("accept") ?? "") + .toLowerCase() + .includes("text/event-stream"); + } + return false; } /** - * A fetch that sends MCP-endpoint requests to `mcpFetch` and everything else - * to `oauthFetch`. + * A fetch that sends MCP traffic to `mcpFetch` and everything else to + * `oauthFetch`. * * @param isMcpEndpoint - `exemptMcpEndpoint(...)`, or any predicate of the - * same shape; `true` means MCP traffic. + * same shape; `true` means the request is addressed to the MCP endpoint. */ export function routeOAuthRequests( mcpFetch: typeof fetch, oauthFetch: typeof fetch, isMcpEndpoint: (url: string, headers: Headers) => boolean, ): typeof fetch { - return (input, init) => { - const url = requestUrlOf(input); - return !isWellKnownUri(url) && - isMcpEndpoint(url, requestHeadersOf(input, init)) + return (input, init) => + isMcpShapedRequest(input, init) && + isMcpEndpoint(requestUrlOf(input), requestHeadersOf(input, init)) ? mcpFetch(input, init) : oauthFetch(input, init); - }; }