diff --git a/mcp/src/bin/http.ts b/mcp/src/bin/http.ts index d1498fb4..7c6dd200 100644 --- a/mcp/src/bin/http.ts +++ b/mcp/src/bin/http.ts @@ -9,6 +9,7 @@ interface BinConfig { maxSessions: number; publicUrl?: string; authorizationServerUrl?: string; + trustProxy: boolean | number | string; } class ConfigError extends Error {} @@ -32,6 +33,21 @@ function parsePort(name: string, raw: string, def: number): number { return n; } +// parseTrustProxy maps E2A_TRUST_PROXY to the value Express's `trust +// proxy` setting expects. Default "loopback": only a same-host proxy (the +// prod Caddy front, forwarding over localhost) is trusted for +// X-Forwarded-* headers. "true"/"false" become booleans; a bare integer +// is a hop count (Express reads a numeric *string* as a subnet, so it must +// be converted); anything else passes through as a preset name +// ("loopback"/"uniquelocal"/...) or a CSV of IPs/subnets. +function parseTrustProxy(raw: string): boolean | number | string { + if (raw === "") return "loopback"; + if (raw === "true") return true; + if (raw === "false") return false; + if (/^\d+$/.test(raw)) return Number(raw); + return raw; +} + function parseHostList(raw: string, def: string): string[] { const source = raw === "" ? def : raw; const list = source.split(",").map((s) => s.trim()).filter(Boolean); @@ -72,6 +88,7 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): BinConfig { maxSessions: parsePositiveInt("MCP_MAX_SESSIONS", env.MCP_MAX_SESSIONS ?? "", 500), publicUrl: env.MCP_PUBLIC_URL || undefined, authorizationServerUrl: env.MCP_AUTHORIZATION_SERVER_URL || undefined, + trustProxy: parseTrustProxy(env.E2A_TRUST_PROXY ?? ""), }; } @@ -96,6 +113,7 @@ async function main(): Promise { maxSessions: cfg.maxSessions, publicUrl: cfg.publicUrl, authorizationServerUrl: cfg.authorizationServerUrl, + trustProxy: cfg.trustProxy, }); process.stderr.write( `e2a-mcp-http listening on :${bound} (base ${cfg.baseUrl}, hosts ${cfg.allowedHosts.join(",")})\n`, diff --git a/mcp/src/http-server.ts b/mcp/src/http-server.ts index 488d1e16..e64d03a5 100644 --- a/mcp/src/http-server.ts +++ b/mcp/src/http-server.ts @@ -32,6 +32,17 @@ export interface HttpServerOptions { * without changing where the SDK forwards bearer tokens. */ authorizationServerUrl?: string; + /** + * Express `trust proxy` setting. Controls which hops' `X-Forwarded-*` + * headers are honored when computing the externally-visible scheme for + * the OAuth discovery URLs (see {@link externalScheme}). Defaults to + * `"loopback"`: only a same-host proxy — the prod Caddy front, which + * forwards to this server over localhost — is trusted, so an + * `X-Forwarded-Proto` spoofed over the public network is ignored. + * Accepts any value Express supports: a preset (`"loopback"`, + * `"uniquelocal"`), a boolean, a hop count, or a CSV of IPs/subnets. + */ + trustProxy?: boolean | number | string; /** Optional shared sessions instance (defaults to a fresh one). */ sessions?: Sessions; /** Session idle timeout. */ @@ -72,6 +83,13 @@ export function buildApp(opts: HttpServerOptions): BuiltApp { }); const app = express(); + // Trust the front proxy so req.protocol reflects X-Forwarded-Proto when + // synthesizing the externally-advertised discovery URLs. Default + // "loopback" only honors the header from a same-host proxy (the prod + // Caddy front, which forwards over localhost); a header arriving over the + // public network is from an untrusted hop and is ignored. Must be set + // before any route that reads req.protocol. + app.set("trust proxy", opts.trustProxy ?? "loopback"); app.use(express.json({ limit: "1mb" })); // CORS open for v0.2; revisit when we have a real allowlist of MCP hosts. app.use(cors({ origin: "*", exposedHeaders: ["Mcp-Session-Id"] })); @@ -121,7 +139,7 @@ export function buildApp(opts: HttpServerOptions): BuiltApp { if (!host) return null; const bare = host.split(":")[0]!.toLowerCase(); if (!allowedHosts.has(bare)) return null; - return `https://${host}`; + return `${externalScheme(req)}://${host}`; }; app.get("/.well-known/oauth-protected-resource", (req, res) => { @@ -153,13 +171,26 @@ export function buildApp(opts: HttpServerOptions): BuiltApp { return { app, sessions }; } +// externalScheme returns the client-facing scheme ("https"/"http") for +// building the OAuth discovery URLs this server advertises. req.protocol +// honors X-Forwarded-Proto only for hops trusted via the `trust proxy` +// setting in buildApp; otherwise it reports the real (plaintext localhost) +// socket scheme. Behind the prod Caddy front this is the forwarded +// "https"; for an untrusted/spoofed header it falls back to the actual +// connection scheme rather than the attacker's claim. Deployments that +// terminate TLS at a non-loopback proxy set publicUrl or E2A_TRUST_PROXY. +function externalScheme(req: Request): string { + return req.protocol === "https" ? "https" : "http"; +} + // resourceMetadataURL returns the value the `WWW-Authenticate` header // should advertise. Honors publicUrl when set (local-dev http), falls -// back to https+Host otherwise (prod behind Caddy). +// back to the request's external scheme + Host otherwise (prod behind +// Caddy resolves to https via X-Forwarded-Proto). function resourceMetadataURL(req: Request, opts: HttpServerOptions): string { const base = opts.publicUrl ? opts.publicUrl.replace(/\/+$/, "") - : `https://${req.headers.host}`; + : `${externalScheme(req)}://${req.headers.host}`; return `${base}/.well-known/oauth-protected-resource`; } diff --git a/mcp/src/session.ts b/mcp/src/session.ts index 319b1553..7e09da0a 100644 --- a/mcp/src/session.ts +++ b/mcp/src/session.ts @@ -132,8 +132,16 @@ export class Sessions { } } if (oldestId) { - // Fire-and-forget; the LRU eviction shouldn't block put(). - void this.delete(oldestId); + // Fire-and-forget; the LRU eviction shouldn't block put(). Attach a + // catch so a rejecting transport.close() surfaces as a logged warning + // rather than an unhandled promise rejection (which can terminate the + // process under Node's default unhandledRejection behavior) — the + // sibling gc()/shutdown() paths get the same protection via + // Promise.allSettled. + void this.delete(oldestId).catch((err) => { + const message = err instanceof Error ? err.message : String(err); + process.stderr.write(`session evict close error: ${message}\n`); + }); } } } diff --git a/mcp/tests/bin-http.test.ts b/mcp/tests/bin-http.test.ts index 5cd65f88..7c6b3362 100644 --- a/mcp/tests/bin-http.test.ts +++ b/mcp/tests/bin-http.test.ts @@ -10,6 +10,7 @@ describe("bin/http loadConfig", () => { allowedHosts: ["mcp.e2a.dev"], sessionIdleMs: 5 * 60_000, maxSessions: 500, + trustProxy: "loopback", }); }); @@ -27,6 +28,7 @@ describe("bin/http loadConfig", () => { allowedHosts: ["mcp.e2a.dev", "mcp-staging.e2a.dev"], sessionIdleMs: 60_000, maxSessions: 100, + trustProxy: "loopback", }); }); @@ -83,4 +85,24 @@ describe("bin/http loadConfig", () => { const cfg = loadConfig({ PORT: "0" }); expect(cfg.port).toBe(0); }); + + it("defaults E2A_TRUST_PROXY to loopback", () => { + expect(loadConfig({}).trustProxy).toBe("loopback"); + }); + + it("parses E2A_TRUST_PROXY booleans", () => { + expect(loadConfig({ E2A_TRUST_PROXY: "true" }).trustProxy).toBe(true); + expect(loadConfig({ E2A_TRUST_PROXY: "false" }).trustProxy).toBe(false); + }); + + it("parses a bare integer E2A_TRUST_PROXY as a hop count", () => { + // Express reads a numeric *string* as a subnet, so it must become a + // real number to mean "trust N hops". + expect(loadConfig({ E2A_TRUST_PROXY: "1" }).trustProxy).toBe(1); + }); + + it("passes through a preset / subnet E2A_TRUST_PROXY verbatim", () => { + expect(loadConfig({ E2A_TRUST_PROXY: "uniquelocal" }).trustProxy).toBe("uniquelocal"); + expect(loadConfig({ E2A_TRUST_PROXY: "10.0.0.0/8" }).trustProxy).toBe("10.0.0.0/8"); + }); }); diff --git a/mcp/tests/http.test.ts b/mcp/tests/http.test.ts index f2e66f74..890b60ac 100644 --- a/mcp/tests/http.test.ts +++ b/mcp/tests/http.test.ts @@ -243,6 +243,48 @@ describe("HTTP MCP server", () => { expect(body.bearer_methods_supported).toEqual(["header"]); }); + it("discovery scheme falls back to the connection scheme without X-Forwarded-Proto", async () => { + const origin = url.replace("/mcp", ""); + const res = await fetch(`${origin}/.well-known/oauth-protected-resource`); + expect(res.status).toBe(200); + const body = await res.json(); + // No forwarded header: advertise the real (plaintext loopback) scheme. + expect(body.resource).toBe(origin); + }); + + it("honors X-Forwarded-Proto from a trusted (loopback) proxy", async () => { + // The default server trusts "loopback"; the test client connects over + // 127.0.0.1, so its X-Forwarded-Proto is honored — mirroring the prod + // Caddy front that terminates TLS and forwards over localhost. + const origin = url.replace("/mcp", ""); + const res = await fetch(`${origin}/.well-known/oauth-protected-resource`, { + headers: { "X-Forwarded-Proto": "https" }, + }); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.resource).toBe(origin.replace(/^http:/, "https:")); + }); + + it("ignores X-Forwarded-Proto when the proxy is not trusted", async () => { + // trustProxy:false simulates an untrusted hop (e.g. a header spoofed + // over the public network rather than set by the loopback front proxy). + // The forwarded scheme must be dropped, not reflected into the resource. + await close(); + const { close: c, port } = await startHttpServer(0, { + baseUrl: "http://e2a.local", + allowedHosts: ["127.0.0.1", "localhost"], + trustProxy: false, + clientFactory: () => stub, + }); + close = c; + const res = await fetch(`http://127.0.0.1:${port}/.well-known/oauth-protected-resource`, { + headers: { "X-Forwarded-Proto": "https" }, + }); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.resource).toBe(`http://127.0.0.1:${port}`); + }); + it("lists every registered tool after initialize", async () => { const { client, transport } = await connect(); const { tools } = await client.listTools(); diff --git a/mcp/tests/session.test.ts b/mcp/tests/session.test.ts index 23dbf917..76210883 100644 --- a/mcp/tests/session.test.ts +++ b/mcp/tests/session.test.ts @@ -59,6 +59,28 @@ describe("Sessions", () => { expect(sessions.get("c")).toBeDefined(); }); + it("LRU eviction tolerates a close() that rejects", async () => { + let t = 0; + const sessions = new Sessions({ idleTimeoutMs: 60_000, maxSessions: 1, now: () => t }); + const stderr = vi.spyOn(process.stderr, "write").mockReturnValue(true); + const bad = { + transport: { close: vi.fn(async () => { throw new Error("boom"); }) }, + server: {}, + lastSeen: 0, + } as unknown as SessionEntry; + sessions.put("bad", bad); + t = 10; + // Evicting "bad" fire-and-forgets a delete() whose close() rejects; this + // must not surface as an unhandled rejection, and the entry is still gone. + sessions.put("good", fakeEntry(10)); + await new Promise((r) => setImmediate(r)); + expect(bad.transport.close).toHaveBeenCalledOnce(); + expect(sessions.get("bad")).toBeUndefined(); + expect(sessions.get("good")).toBeDefined(); + expect(stderr).toHaveBeenCalledWith(expect.stringContaining("session evict close error: boom")); + stderr.mockRestore(); + }); + it("re-putting an existing id does not trigger eviction", () => { const sessions = new Sessions({ idleTimeoutMs: 60_000, maxSessions: 1 }); const a1 = fakeEntry();