From f5c647350303f3c985a513235cb384b78f8cabe7 Mon Sep 17 00:00:00 2001 From: Dan Carter Date: Thu, 13 Aug 2026 19:26:10 -0400 Subject: [PATCH] Harden MCP portal authentication changes --- .../__tests__/account-revision.test.ts | 54 ++++++++++++ .../__tests__/config.test.ts | 8 +- packages/gatekeeper-mcp-portal/src/config.ts | 2 +- packages/gatekeeper-mcp-portal/src/portal.ts | 85 ++++++++++++++++++- .../gatekeeper-mcp-portal/vitest.config.ts | 13 +++ packages/mcp-shared/src/account.ts | 43 ++++++++-- 6 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts create mode 100644 packages/gatekeeper-mcp-portal/vitest.config.ts diff --git a/packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts b/packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts new file mode 100644 index 00000000..23c1298a --- /dev/null +++ b/packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts @@ -0,0 +1,54 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + McpAccountBase, + type ConnectedServer, + type ConnectOutcome, +} from "@gadgets/mcp-shared/account"; +import { McpAccount } from "../src/portal.js"; + +afterEach(() => vi.restoreAllMocks()); + +describe("McpAccount configuration revision", () => { + it("records the current revision before handing an OAuth attempt to its callback", async () => { + const values = new Map([["portalConfigRevision", "old"]]); + const ctx = { + id: { toString: () => "account" }, + storage: { + kv: { + get: (key: string) => values.get(key), + put: (key: string, value: unknown) => values.set(key, value), + delete: (key: string) => values.delete(key), + }, + }, + exports: {}, + }; + const env = { + MCP_PORTAL_URL: "https://portal.example.com/mcp", + MCP_PORTAL_AUTH: "oauth", + }; + const server: ConnectedServer = { + endpoint: env.MCP_PORTAL_URL, + serverId: "portal", + serverName: "Portal", + provenance: "deployment", + auth: "oauth", + }; + const base = McpAccountBase.prototype as unknown as { + beginConnect(nonce: string, target: ConnectedServer | null): Promise; + }; + vi.spyOn(base, "beginConnect") + .mockResolvedValue({ kind: "redirect", url: "https://login.example.com" }); + const account = new McpAccount(ctx as never, env as never); + const internals = account as unknown as { + awaitingSelection(nonce: string): boolean; + server(): ConnectedServer | undefined; + }; + vi.spyOn(internals, "awaitingSelection").mockReturnValue(true); + vi.spyOn(internals, "server").mockReturnValue(server); + + await expect(account.beginConnect("nonce", server)).resolves.toMatchObject({ kind: "redirect" }); + + expect(values.get("portalConfigRevision")).not.toBe("old"); + }); +}); diff --git a/packages/gatekeeper-mcp-portal/__tests__/config.test.ts b/packages/gatekeeper-mcp-portal/__tests__/config.test.ts index f8a8813e..03218f17 100644 --- a/packages/gatekeeper-mcp-portal/__tests__/config.test.ts +++ b/packages/gatekeeper-mcp-portal/__tests__/config.test.ts @@ -140,9 +140,13 @@ describe("portalAuthRequiresReconnect", () => { expect(portalAuthRequiresReconnect("token", "oauth")).toBe(true); }); - it("allows none and oauth to differ after probing the endpoint", () => { + it("allows an oauth-configured portal to prove public during probing", () => { expect(portalAuthRequiresReconnect("none", "oauth")).toBe(false); - expect(portalAuthRequiresReconnect("oauth", "none")).toBe(false); + }); + + it("keeps explicitly unauthenticated mode strict", () => { + expect(portalAuthRequiresReconnect("oauth", "none")).toBe(true); + expect(portalAuthRequiresReconnect("none", "none")).toBe(false); }); }); diff --git a/packages/gatekeeper-mcp-portal/src/config.ts b/packages/gatekeeper-mcp-portal/src/config.ts index f1e57699..6b509f2a 100644 --- a/packages/gatekeeper-mcp-portal/src/config.ts +++ b/packages/gatekeeper-mcp-portal/src/config.ts @@ -169,7 +169,7 @@ export function portalServer(config: PortalConfig): ConnectedServer { export function portalAuthRequiresReconnect( connected: ServerAuthKind, configured: ServerAuthKind, ): boolean { - return (connected === "token") !== (configured === "token"); + return configured === "oauth" ? connected === "token" : connected !== configured; } /** diff --git a/packages/gatekeeper-mcp-portal/src/portal.ts b/packages/gatekeeper-mcp-portal/src/portal.ts index f4ba44d2..7f971e40 100644 --- a/packages/gatekeeper-mcp-portal/src/portal.ts +++ b/packages/gatekeeper-mcp-portal/src/portal.ts @@ -28,7 +28,7 @@ import { type ToolIndex, } from "@gadgets/mcp-shared/client"; import { MAX_TOOLS_PER_SERVER, type ServerTrust } from "@gadgets/mcp-shared/tools"; -import { bindingNameFragment, hostOf } from "@gadgets/mcp-shared/util"; +import { bindingNameFragment, hexEncode, hostOf } from "@gadgets/mcp-shared/util"; import type { McpLog, McpLogFields } from "@gadgets/mcp-shared/log"; import { generateSessionTypes, sessionTypeName } from "@gadgets/mcp-shared/schema-to-ts"; import { McpAccountBase, type ConnectedServer, type ConnectOutcome } @@ -38,6 +38,7 @@ import { fetchToolIndex, withClient, type ConnectionAccount, + type McpConnection, } from "@gadgets/mcp-shared/connection"; import { McpSessionBase } from "@gadgets/mcp-shared/session"; import { McpFacetBase } from "@gadgets/mcp-shared/facet"; @@ -79,6 +80,7 @@ import { readPortalConfig, requirePortalServerScope, toolGrantOptions, + type PortalConfig, } from "./config.js"; import type { ConfiguratorUIOption } from "@gadgets/configurator-ui"; import { MCP_BASE_TYPES } from "@gadgets/mcp-shared/base-types"; @@ -247,6 +249,10 @@ export class GatekeeperVendor extends WorkerEntrypoint implements Gatekeepe * real addition: a portal may be fronted by one instead of using OAuth. */ export class McpAccount extends McpAccountBase { + // Counts in-flight attempts per revision. Separate Durable Object requests can overlap at the + // digest and portal probe, so one losing or older attempt must not clear a newer attempt's guard. + #connectingRevisions = new Map(); + protected baseUrl(): string { return getBaseUrl(this.env); } @@ -267,6 +273,83 @@ export class McpAccount extends McpAccountBase { protected override staticToken(server: ConnectedServer): string | null { return portalTokenFor(this.env, server.endpoint); } + + protected override allowsOAuthFallback(server: ConnectedServer): boolean { + return server.auth !== "none"; + } + + protected override allowsOAuthCallback(server: ConnectedServer): boolean { + const config = readPortalConfig(this.env); + return config?.auth === "oauth" && sameEndpoint(config.endpoint, server.endpoint); + } + + async #configurationRevision(config: PortalConfig): Promise { + const token = config.auth === "token" ? this.env.MCP_PORTAL_TOKEN ?? "" : ""; + const digest = await crypto.subtle.digest( + "SHA-256", + new TextEncoder().encode(`${config.auth}\u0000${token}`), + ); + return hexEncode(new Uint8Array(digest)); + } + + override async beginConnect( + initiationNonce: string, + target: ConnectedServer | null, + ): Promise { + const config = readPortalConfig(this.env); + const revision = config && this.awaitingSelection(initiationNonce) + ? await this.#configurationRevision(config) + : undefined; + if (revision !== undefined) { + this.#connectingRevisions.set( + revision, (this.#connectingRevisions.get(revision) ?? 0) + 1); + } + try { + const outcome = await super.beginConnect(initiationNonce, target); + if (outcome.kind !== "invalid") { + const current = readPortalConfig(this.env); + const server = this.server(); + if (current && server && sameEndpoint(current.endpoint, server.endpoint)) { + this.ctx.storage.kv.put( + "portalConfigRevision", + await this.#configurationRevision(current), + ); + } + } + return outcome; + } finally { + if (revision !== undefined) { + const remaining = (this.#connectingRevisions.get(revision) ?? 1) - 1; + if (remaining === 0) this.#connectingRevisions.delete(revision); + else this.#connectingRevisions.set(revision, remaining); + } + } + } + + override async getConnection(endpoint: string): Promise { + const config = readPortalConfig(this.env); + const server = this.server(); + if (!config || !server || !sameEndpoint(config.endpoint, endpoint) + || portalAuthRequiresReconnect(server.auth, config.auth)) { + throw new Error("This deployment's MCP portal configuration changed. Reconnect the account."); + } + const revision = await this.#configurationRevision(config); + const previous = this.ctx.storage.kv.get("portalConfigRevision"); + const reconnectingToCurrentRevision = this.#connectingRevisions.has(revision); + if (previous === undefined) { + // Existing token accounts predate the revision marker. Their cached session may have been + // minted under a different configured token, so invalidate it once before establishing the + // current revision as the baseline. New connects record the revision in `beginConnect()`. + if (!reconnectingToCurrentRevision) { + if (server.auth === "token") this.invalidateConnectionState(); + this.ctx.storage.kv.put("portalConfigRevision", revision); + } + } else if (previous !== revision && !reconnectingToCurrentRevision) { + this.invalidateConnectionState(); + this.ctx.storage.kv.put("portalConfigRevision", revision); + } + return super.getConnection(endpoint); + } } // --------------------------------------------------------------------------- diff --git a/packages/gatekeeper-mcp-portal/vitest.config.ts b/packages/gatekeeper-mcp-portal/vitest.config.ts new file mode 100644 index 00000000..48495a37 --- /dev/null +++ b/packages/gatekeeper-mcp-portal/vitest.config.ts @@ -0,0 +1,13 @@ +import { fileURLToPath } from "node:url"; +import capnwebValidate from "capnweb-validate/vite"; +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + plugins: [capnwebValidate()], + test: { + alias: { + "cloudflare:workers": fileURLToPath( + new URL("../mcp-shared/__tests__/stubs/cloudflare-workers.ts", import.meta.url)), + }, + }, +}); diff --git a/packages/mcp-shared/src/account.ts b/packages/mcp-shared/src/account.ts index 937396eb..85b676e2 100644 --- a/packages/mcp-shared/src/account.ts +++ b/packages/mcp-shared/src/account.ts @@ -180,6 +180,16 @@ export abstract class McpAccountBase return null; } + /** Whether an unauthenticated target may follow a 401 into the standard OAuth flow. */ + protected allowsOAuthFallback(_server: ConnectedServer): boolean { + return true; + } + + /** Whether current connector configuration still permits this pending OAuth callback. */ + protected allowsOAuthCallback(_server: ConnectedServer): boolean { + return true; + } + /** Relaxes host and scheme checks for local development against an MCP server on localhost. */ protected fetchOptions(): FetchOptions { return fetchOptions(this.env); @@ -203,6 +213,13 @@ export abstract class McpAccountBase return generation; } + /** Invalidates credentials captured by facets and clears their transport session. */ + protected invalidateConnectionState(): void { + this.advanceConnectionGeneration(); + this.ctx.storage.kv.delete("mcpSessionId"); + this.ctx.storage.kv.put("expiredNotified", false); + } + private isCurrentConnection(server: ConnectedServer, generation: number): boolean { const current = this.server(); return this.connectionGeneration() === generation && current !== undefined && @@ -310,7 +327,8 @@ export abstract class McpAccountBase const generation = this.advanceConnectionGeneration(); if (existing) this.ctx.storage.kv.delete("mcpSessionId"); const endpointChanged = existing !== undefined && existing.endpoint !== server.endpoint; - if (endpointChanged) { + const authChanged = existing !== undefined && existing.auth !== server.auth; + if (endpointChanged || authChanged) { this.ctx.storage.kv.put("server", server); for (const key of [ "tokens", "oauthClient", "oauthDiscovery", "oauthVerifier", "pendingAuth", @@ -318,10 +336,12 @@ export abstract class McpAccountBase this.ctx.storage.kv.delete(key); } this.ctx.storage.kv.put("expiredNotified", false); - this.log().info("portal repointed", { - event: "connect.repointed", - serverHost: hostOf(server.endpoint), - }); + if (endpointChanged) { + this.log().info("portal repointed", { + event: "connect.repointed", + serverHost: hostOf(server.endpoint), + }); + } } const log = this.log().with({ @@ -374,6 +394,13 @@ export abstract class McpAccountBase `The MCP server "${server.serverName}" rejected this deployment's configured token.`, { cause: err }); } + if (!this.allowsOAuthFallback(server)) { + this.restoreSelection(initiationNonce); + throw new Error( + `The MCP server "${server.serverName}" requires authorization, but this connection is ` + + "configured for unauthenticated access.", + { cause: err }); + } // The endpoint answered with an authorization challenge, so OAuth is now the observed auth // mode even if deployment configuration optimistically called the portal public. Persist that // mode because `getAuthorization()` uses it to decide whether to read the tokens the callback @@ -577,6 +604,12 @@ export abstract class McpAccountBase const pending = this.ctx.storage.kv.get("pendingAuth"); if (!pending) return false; const server = this.requireServer(); + if (!this.allowsOAuthCallback(server)) { + this.ctx.storage.kv.delete("nonce"); + this.ctx.storage.kv.delete("pendingAuth"); + this.ctx.storage.kv.delete("oauthVerifier"); + return false; + } // Single-use: consumed before the exchange, so a replayed callback cannot reach the token endpoint. this.ctx.storage.kv.delete("nonce"); this.ctx.storage.kv.delete("pendingAuth");