diff --git a/packages/gatekeeper-mcp-portal/__tests__/reconnect.test.ts b/packages/gatekeeper-mcp-portal/__tests__/reconnect.test.ts new file mode 100644 index 000000000..a4bf8bc82 --- /dev/null +++ b/packages/gatekeeper-mcp-portal/__tests__/reconnect.test.ts @@ -0,0 +1,99 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { GatekeeperUserImpl } from "../src/portal.js"; + +// Enough of `McpAccount` for `withClient` to run a `tools/call`, plus the base reconnect account +// methods so a fallback to the inherited flow would be observable. +function accountStub() { + return { + async getServer() { + return { + endpoint: "https://gw.example.com/mcp", + serverId: "gw-example", + serverName: "Portal", + provenance: "deployment" as const, + auth: "oauth" as const, + }; + }, + async getConnection() { + // A session id already on file skips `initialize`, so the stubbed `fetch` below only has to + // answer the one `tools/call` request this test cares about. + return { authorization: "token", sessionId: "session", generation: 1 }; + }, + async assertConnectionCurrent() {}, + async setMcpSessionId() { return true; }, + async noteCredentialsExpired() {}, + prepareReconnect: vi.fn(async () => {}), + revoke: vi.fn(async () => {}), + }; +} + +function user(account: ReturnType) { + const ctx = { + props: { accountObjectId: "account-1" }, + exports: { + McpAccount: { + idFromString: (id: string) => id, + get: () => account, + }, + }, + }; + return new GatekeeperUserImpl(ctx as never, {} as never); +} + +function jsonRpcResult(id: unknown, result: unknown) { + return new Response(JSON.stringify({ jsonrpc: "2.0", id, result }), { + headers: { "Content-Type": "application/json" }, + }); +} + +afterEach(() => vi.unstubAllGlobals()); + +describe("reconnect", () => { + it("calls portal_toggle_servers with no arguments and returns the URL it finds", async () => { + let toolCallParams: unknown; + vi.stubGlobal("fetch", async (_input: unknown, init?: RequestInit) => { + const request = JSON.parse(String(init?.body)); + toolCallParams = request.params; + return jsonRpcResult(request.id, { + content: [{ type: "text", text: "Re-authenticate: https://gw.example.com/reauth?x=1" }], + }); + }); + + const { url } = await user(accountStub()).reconnect(); + + expect(url).toBe("https://gw.example.com/reauth?x=1"); + expect(toolCallParams).toEqual({ name: "portal_toggle_servers", arguments: {} }); + }); + + it("throws instead of falling back to the inherited own-OAuth reconnect when no URL is found", async () => { + // The base `McpGatekeeperUserBase.reconnect()` this overrides calls `account.prepareReconnect`. + // If this override ever fell back to it on failure, the bug it fixes would return: the user + // reconnects, the upstream authorization is untouched, and the error comes right back. + vi.stubGlobal("fetch", async (_input: unknown, init?: RequestInit) => { + const request = JSON.parse(String(init?.body)); + return jsonRpcResult(request.id, { + content: [{ type: "text", text: "Nothing usable here." }], + }); + }); + + const account = accountStub(); + await expect(user(account).reconnect()).rejects.toThrow( + "Could not get a re-authentication URL from the portal."); + expect(account.prepareReconnect).not.toHaveBeenCalled(); + }); + + it("throws on a tool-level error without leaking the response into the user-facing message", async () => { + vi.stubGlobal("fetch", async (_input: unknown, init?: RequestInit) => { + const request = JSON.parse(String(init?.body)); + return jsonRpcResult(request.id, { + isError: true, + content: [{ type: "text", text: "internal detail https://leak.example.com/should-not-surface" }], + }); + }); + + const error: Error = await user(accountStub()).reconnect().catch(err => err); + expect(error).toBeInstanceOf(Error); + expect(error.message).not.toContain("leak.example.com"); + }); +}); diff --git a/packages/gatekeeper-mcp-portal/__tests__/stubs/cloudflare-workers.ts b/packages/gatekeeper-mcp-portal/__tests__/stubs/cloudflare-workers.ts new file mode 100644 index 000000000..c2de84240 --- /dev/null +++ b/packages/gatekeeper-mcp-portal/__tests__/stubs/cloudflare-workers.ts @@ -0,0 +1,20 @@ +// Stand-in for the `cloudflare:workers` module, which only exists inside workerd. +// +// Modules here that define a Durable Object or an `RpcTarget` cannot be imported under plain vitest +// without it. Only the base classes are provided, and only so that `import` and `extends` resolve -- +// anything that actually needs the runtime belongs in a Workers-pool test, not here. Mirrors +// `@gadgets/mcp-shared`'s stub of the same name. + +export class DurableObject { + constructor(readonly ctx: unknown, readonly env: E, readonly props?: P) {} +} + +export class RpcTarget {} + +export class WorkerEntrypoint { + constructor(readonly ctx: unknown, readonly env: E, readonly props?: P) {} +} + +export class RpcStub { + constructor(readonly target: T) {} +} diff --git a/packages/gatekeeper-mcp-portal/src/portal.ts b/packages/gatekeeper-mcp-portal/src/portal.ts index ea7374b7a..e7ad20187 100644 --- a/packages/gatekeeper-mcp-portal/src/portal.ts +++ b/packages/gatekeeper-mcp-portal/src/portal.ts @@ -23,7 +23,7 @@ import { type SupportedResource, type VendorDescription, } from "@gadgets/workshop-shared/gatekeeper"; -import { isValidToolName } from "@gadgets/mcp-shared/client"; +import { isValidToolName, 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 type { McpLog, McpLogFields } from "@gadgets/mcp-shared/log"; @@ -36,8 +36,10 @@ import { McpSessionBase } from "@gadgets/mcp-shared/session"; import { McpFacetBase } from "@gadgets/mcp-shared/facet"; import { looksLikePortal, + parsePortalReconnectUrl, parsePortalServers, PORTAL_LIST_SERVERS_TOOL, + PORTAL_TOGGLE_SERVERS_TOOL, reconcilePortalServers, type PortalServer, type PortalServerListing, @@ -136,6 +138,125 @@ async function tryListPortalServers( } } +/** + * Validates one portal-scoped resource URL, returning the scope it grants together with the upstream + * server that scope names, when the portal reported one. Throws if the scope is not grantable. + * + * The fragment records how much of the portal this binding may call; see `scope.ts`. A grant that + * names no upstream server would reach every system behind the portal, so it is refused here rather + * than only in the form that normally builds these URLs. + * + * The server-list result is advisory metadata, so failing to obtain it is not fatal on its own. But + * the endpoint still has to prove it implements the portal capability before a portal-scoped binding + * can be minted, which is what the `findTool` probe below establishes. + * + * Each mode then fetches only the names validation still needs. Named grants prove each selected + * name. A reported server needs no catalog scan; an unreported server needs one prefixed tool as + * fallback evidence. + */ +async function validatePortalScope( + env: Env, + account: DurableObjectStub, + endpoint: string, + requested: URL, +): Promise<{ scope: ToolScope & { serverId: string }; upstream: PortalServer | undefined }> { + const scope = parseToolScope(requested); + requirePortalServerScope(scope); + + const listing = await tryListPortalServers(env, account, endpoint); + if (listing === null) { + const portalTool = await withClient(env, account, endpoint, + client => client.findTool(PORTAL_LIST_SERVERS_TOOL)); + if (!portalTool) { + throw new Error("The configured MCP endpoint does not expose the portal server-list tool."); + } + } + + const servers = listing?.servers ?? []; + const requestedTools = new Set(scope.tools ?? []); + let catalog: ToolIndex; + switch (portalCatalogValidationMode(scope, servers)) { + case "named-tools": + catalog = await withClient(env, account, endpoint, + client => client.listMatchingToolIndex( + requestedTools.size, + tool => requestedTools.has(tool.name), + )); + break; + case "reported-server": + catalog = { tools: [], truncated: false }; + break; + case "server-evidence": + catalog = await withClient(env, account, endpoint, + client => client.listMatchingToolIndex( + 1, + tool => isPortalToolGrantable(tool.name, scope.serverId), + )); + break; + } + return { scope, upstream: validateToolScopeAgainstCatalog(scope, catalog, servers) }; +} + +/** + * The servers behind the portal, for the configurator's picker. Returns an empty list when the + * endpoint is not a portal at all, but throws when it is one whose server list could not be read + * completely: an incomplete picker would silently hide servers the user is entitled to grant. + */ +async function listAvailablePortalServers( + env: Env, + account: DurableObjectStub, + endpoint: string, +): Promise { + const reported = await tryListPortalServers(env, account, endpoint); + if (reported?.complete) return reported.servers; + + const index = await withClient(env, account, endpoint, + client => client.listToolIndex(MAX_PORTAL_TOOL_INDEX)); + if (!looksLikePortal( + index.tools, { truncated: index.truncated, cap: MAX_PORTAL_TOOL_INDEX })) return []; + if (index.truncated) { + throw new Error("Could not retrieve the portal's complete server list. Try again."); + } + return reconcilePortalServers(reported?.servers ?? [], index.tools); +} + +/** + * Opens the portal's own re-authentication page, via `portal_toggle_servers`. Unlike + * `getSupportedResources`'s reconnect story, this is not the deployment's OAuth with Cloudflare + * Access; it is the portal's on-behalf authorization to an upstream server, and + * `portal_toggle_servers` is the only path Cloudflare's MCP Server Portals expose to recover it -- + * there is no dashboard alternative. + * + * Called with no arguments: the tool is documented only as "opens a URL-based server selection + * page", with nothing suggesting a required argument, and the sibling `portal_list_servers` takes + * none either. + * + * The response shape is undocumented, so the URL is recovered by the same permissive prose scan as + * `portal_list_servers` rather than trusting a guessed structured field. When no URL can be found, + * the full response is logged for later inspection but never folded into the thrown message -- + * its shape is unverified, so it must not reach the user-facing error. + */ +async function reconnectPortal( + env: Env, + account: DurableObjectStub, + endpoint: string, +): Promise { + const result = await withClient(env, account, endpoint, + client => client.callTool(PORTAL_TOGGLE_SERVERS_TOOL, {})); + const url = result.isError ? null : parsePortalReconnectUrl(result); + if (!url) { + // The raw response shape is unverified, so it goes through `error` (logged, never surfaced) + // rather than into the thrown message below, which reaches the user. + logger.warn("could not recover a reconnect URL from the portal", { + event: "portal.reconnect.url.missing", + serverHost: hostOf(endpoint), + error: JSON.stringify(result), + }); + throw new Error("Could not get a re-authentication URL from the portal."); + } + return url; +} + // HTTP handler. There is no page asking which server to connect, since the endpoint is configured, // so the only browser round trip is the portal's own authorization. @@ -326,46 +447,9 @@ export class GatekeeperUserImpl throw new Error(`"${url}" does not match this connection's resource type.`); } - // The fragment records how much of the portal this binding may call; see `scope.ts`. A grant - // that names no upstream server would reach every system behind the portal, so it is refused - // here rather than only in the form that normally builds these URLs. - const scope = parseToolScope(requested); - requirePortalServerScope(scope); const account = this.#account(); - const listedServers = await tryListPortalServers(this.env, account, server.endpoint); - if (listedServers === null) { - // The server-list result is advisory metadata, but the endpoint still has to prove it implements - // the portal capability before a portal-scoped binding can be minted. - const portalTool = await withClient(this.env, account, server.endpoint, - client => client.findTool(PORTAL_LIST_SERVERS_TOOL)); - if (!portalTool) { - throw new Error("The configured MCP endpoint does not expose the portal server-list tool."); - } - } - const portalServers = listedServers?.servers ?? []; - - // Fetch only the names validation still needs. Named grants prove each selected name. A reported - // server needs no catalog scan; an unreported server needs one prefixed tool as fallback evidence. - const requestedTools = new Set(scope.tools ?? []); - const validationMode = portalCatalogValidationMode(scope, portalServers); - const catalog = validationMode === "named-tools" - ? await withClient(this.env, account, server.endpoint, - client => client.listMatchingToolIndex( - requestedTools.size, - tool => requestedTools.has(tool.name), - )) - : validationMode === "reported-server" - ? { tools: [], truncated: false } - : await withClient(this.env, account, server.endpoint, - client => client.listMatchingToolIndex( - 1, - tool => isPortalToolGrantable(tool.name, scope.serverId), - )); - const upstream = validateToolScopeAgainstCatalog( - scope, - catalog, - portalServers, - ); + const { scope, upstream } = await validatePortalScope( + this.env, account, server.endpoint, requested); const props: McpGatekeeperImplProps = { accountObjectId: this.ctx.props.accountObjectId, @@ -378,6 +462,22 @@ export class GatekeeperUserImpl return { class: this.ctx.exports.McpGatekeeperImpl({ props }), resource }; } + /** + * Overrides the inherited `reconnect()`: that base implementation only restarts this + * gatekeeper's own OAuth with Cloudflare Access, and never touches the portal's on-behalf + * authorization to whichever upstream server is failing. When an upstream token lapses, the + * Gadget-facing error tells the user to "Call the portal_toggle_servers tool to + * re-authenticate" -- something only this Worker, not the Gadget, is positioned to do. So this + * calls that tool itself. Falling back to the inherited flow on failure would reproduce the bug + * this fixes: the user reconnects, nothing about the upstream authorization changes, and the + * error returns on the next call. + */ + async reconnect(): Promise<{ url: string }> { + const server = await this.#account().getServer(); + const url = await reconnectPortal(this.env, this.#account(), server.endpoint); + return { url }; + } + async startResourceConfigurator(_resourceUrlPattern: string): Promise { return { iframeHtml: MCP_SERVER_CONFIGURATOR_HTML, @@ -431,17 +531,7 @@ class McpServerConfiguratorUI extends RpcTarget implements McpServerConfigurator #portalServers(): Promise { return this.#portalServersPromise ??= (async () => { const server = await this.#server(); - const reported = await tryListPortalServers(this.#env, this.#account, server.endpoint); - if (reported?.complete) return reported.servers; - - const index = await withClient(this.#env, this.#account, server.endpoint, - client => client.listToolIndex(MAX_PORTAL_TOOL_INDEX)); - if (!looksLikePortal( - index.tools, { truncated: index.truncated, cap: MAX_PORTAL_TOOL_INDEX })) return []; - if (index.truncated) { - throw new Error("Could not retrieve the portal's complete server list. Try again."); - } - return reconcilePortalServers(reported?.servers ?? [], index.tools); + return listAvailablePortalServers(this.#env, this.#account, server.endpoint); })(); } diff --git a/packages/gatekeeper-mcp-portal/tsconfig.json b/packages/gatekeeper-mcp-portal/tsconfig.json index a5542c6cf..a486fa073 100644 --- a/packages/gatekeeper-mcp-portal/tsconfig.json +++ b/packages/gatekeeper-mcp-portal/tsconfig.json @@ -10,7 +10,6 @@ "types": ["./worker-configuration.d.ts"], "paths": { "@gadgets/configurator-ui": ["../configurator-ui/src/index.ts"], - "@gadgets/mcp-shared/*": ["../mcp-shared/src/*"], "@gadgets/workshop-shared/*": ["../workshop-shared/src/*"] } }, diff --git a/packages/gatekeeper-mcp-portal/vitest.config.ts b/packages/gatekeeper-mcp-portal/vitest.config.ts new file mode 100644 index 000000000..cf3cb64c2 --- /dev/null +++ b/packages/gatekeeper-mcp-portal/vitest.config.ts @@ -0,0 +1,17 @@ +import { fileURLToPath } from "node:url"; +import capnwebValidate from "capnweb-validate/vite"; +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + plugins: [capnwebValidate()], + test: { + include: ["__tests__/*.test.ts"], + environment: "node", + alias: { + // Lets `src/portal.ts` -- which declares a Durable Object, a `WorkerEntrypoint`, and an + // `RpcTarget` -- be imported at all. See the stub for what it does and does not provide. + "cloudflare:workers": fileURLToPath( + new URL("./__tests__/stubs/cloudflare-workers.ts", import.meta.url)), + }, + }, +}); diff --git a/packages/gatekeeper-mcp/tsconfig.json b/packages/gatekeeper-mcp/tsconfig.json index a5542c6cf..a486fa073 100644 --- a/packages/gatekeeper-mcp/tsconfig.json +++ b/packages/gatekeeper-mcp/tsconfig.json @@ -10,7 +10,6 @@ "types": ["./worker-configuration.d.ts"], "paths": { "@gadgets/configurator-ui": ["../configurator-ui/src/index.ts"], - "@gadgets/mcp-shared/*": ["../mcp-shared/src/*"], "@gadgets/workshop-shared/*": ["../workshop-shared/src/*"] } }, diff --git a/packages/mcp-shared/__tests__/portal.test.ts b/packages/mcp-shared/__tests__/portal.test.ts index 4670a1a27..80d1e89b7 100644 --- a/packages/mcp-shared/__tests__/portal.test.ts +++ b/packages/mcp-shared/__tests__/portal.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { isPortalNativeTool, looksLikePortal, + parsePortalReconnectUrl, parsePortalServers, reconcilePortalServers, toolBelongsToServer, @@ -230,3 +231,40 @@ describe("reconcilePortalServers", () => { ).map(server => server.name)).toEqual(["Asana", "Zulip"]); }); }); + +describe("reconnect URL recovery", () => { + it("recovers a URL embedded in prose", () => { + expect(parsePortalReconnectUrl({ + content: [{ type: "text", text: "Open this to re-authenticate: https://gw.example.com/reauth?x=1" }], + })).toBe("https://gw.example.com/reauth?x=1"); + }); + + it("stops at trailing punctuation a sentence would add", () => { + expect(parsePortalReconnectUrl({ + content: [{ type: "text", text: "See (https://gw.example.com/reauth) for details." }], + })).toBe("https://gw.example.com/reauth"); + }); + + it("joins multiple text blocks before searching", () => { + expect(parsePortalReconnectUrl({ + content: [ + { type: "text", text: "Re-authenticate here:" }, + { type: "text", text: "https://gw.example.com/reauth" }, + ], + })).toBe("https://gw.example.com/reauth"); + }); + + it("returns null when no URL is present, rather than guessing", () => { + expect(parsePortalReconnectUrl({ + content: [{ type: "text", text: "Re-authentication is not currently available." }], + })).toBeNull(); + expect(parsePortalReconnectUrl({ content: [] })).toBeNull(); + expect(parsePortalReconnectUrl({})).toBeNull(); + }); + + it("ignores non-text content blocks", () => { + expect(parsePortalReconnectUrl({ + content: [{ type: "image", data: "https://gw.example.com/should-be-ignored" }], + })).toBeNull(); + }); +}); diff --git a/packages/mcp-shared/package.json b/packages/mcp-shared/package.json index 26ef8d9cd..8b0868b05 100644 --- a/packages/mcp-shared/package.json +++ b/packages/mcp-shared/package.json @@ -5,9 +5,7 @@ "type": "module", "exports": { "./account": "./src/account.ts", - "./action-store": "./src/action-store.ts", "./base-types": "./src/base-types.ts", - "./catalog": "./src/catalog.ts", "./client": "./src/client.ts", "./connect-nonce": "./src/connect-nonce.ts", "./connection": "./src/connection.ts", @@ -17,15 +15,10 @@ "./html": "./src/html.ts", "./http": "./src/http.ts", "./log": "./src/log.ts", - "./oauth": "./src/oauth.ts", - "./oauth-callback": "./src/oauth-callback.ts", "./portal": "./src/portal.ts", "./schema-to-ts": "./src/schema-to-ts.ts", "./scope": "./src/scope.ts", "./session": "./src/session.ts", - "./session-methods": "./src/session-methods.ts", - "./sharing-policy": "./src/sharing-policy.ts", - "./tool-search": "./src/tool-search.ts", "./tools": "./src/tools.ts", "./types": "./src/types.d.ts", "./user": "./src/user.ts", diff --git a/packages/mcp-shared/src/client.ts b/packages/mcp-shared/src/client.ts index 3d5dcfe79..61a0a8ce8 100644 --- a/packages/mcp-shared/src/client.ts +++ b/packages/mcp-shared/src/client.ts @@ -567,7 +567,7 @@ export class McpClient { * tools cannot crowd the requested server or exact grant names out of the bounded result. */ async listTools(maxTools: number, include?: McpToolFilter): Promise { - return this.#list(maxTools, include, clampToolDefinition); + return this.#list({ maxTools, include, project: clampToolDefinition }); } /** @@ -575,42 +575,59 @@ export class McpClient { * definition with `findTool` before use. */ async listToolIndex(maxTools: number): Promise { - return this.#list(maxTools, undefined, indexTool); + return this.#list({ maxTools, project: indexTool }); } /** Collects at most `maxTools` matching index entries without scanning later pages. */ async listMatchingToolIndex(maxTools: number, include: McpToolFilter): Promise { - return this.#list(maxTools, include, indexTool, true); + return this.#list({ maxTools, include, project: indexTool, stopWhenFull: true }); } /** Finds one exact tool without reading pages after the match. */ async findTool(name: string): Promise { if (!isValidToolName(name)) return undefined; - return (await this.#list( - 1, tool => tool.name === name, clampToolDefinition, true, true)).tools[0]; + return (await this.#list({ + maxTools: 1, + include: tool => tool.name === name, + project: clampToolDefinition, + stopWhenFull: true, + requireCompleteScan: true, + })).tools[0]; } /** Collects at most `maxTools` bounded matching summaries without scanning later pages. */ async listMatchingToolSummaries(maxTools: number, include: McpToolFilter): Promise { - return (await this.#list(maxTools, include, clampToolSummary, true, true)).tools; + return (await this.#list({ + maxTools, + include, + project: clampToolSummary, + stopWhenFull: true, + requireCompleteScan: true, + })).tools; } // The shared listing loop. `project` decides how much of each tool is retained, and therefore how // much of the byte budget each one costs; the budget itself is applied to whatever it returns. - async #list( - maxTools: number, - include: McpToolFilter | undefined, - project: (tool: McpWireTool) => T, + async #list({ + maxTools, + include, + project, stopWhenFull = false, - failOnScanLimit = false, - ): Promise<{ tools: T[]; truncated: boolean }> { + requireCompleteScan = false, + }: { + maxTools: number; + include?: McpToolFilter; + project: (tool: McpWireTool) => T; + stopWhenFull?: boolean; + requireCompleteScan?: boolean; + }): Promise<{ tools: T[]; truncated: boolean }> { const tools: T[] = []; let budget = MAX_CATALOG_BYTES; let scannedBytes = 0; let scannedTools = 0; let cursor: string | undefined; const scanLimit = (): { tools: T[]; truncated: boolean } => { - if (failOnScanLimit) { + if (requireCompleteScan) { throw new McpProtocolError( "MCP tool discovery exceeded its scan budget.", undefined, "declined"); } diff --git a/packages/mcp-shared/src/http.ts b/packages/mcp-shared/src/http.ts index f63629aa7..54eda1059 100644 --- a/packages/mcp-shared/src/http.ts +++ b/packages/mcp-shared/src/http.ts @@ -1,13 +1,53 @@ import { stripTrailingSlashes } from "@gadgets/workshop-shared/gatekeeper"; import { NONCE_BYTES } from "./connect-nonce.js"; -import { htmlResponse, INVALID_LINK_HTML } from "./html.js"; +import { + errorPageHtml, + htmlResponse, + INVALID_LINK_HTML, + SELF_CLOSING_HTML, +} from "./html.js"; import type { McpLog } from "./log.js"; -import { handleOAuthCallback } from "./oauth-callback.js"; type OAuthCallbackAccount = { acceptAuthCode(code: string, nonce: string, issuer?: string): Promise; }; +async function handleOAuthCallback( + url: URL, + accountForId: (id: string) => OAuthCallbackAccount, + log: McpLog, +): Promise { + const error = url.searchParams.get("error"); + if (error) { + const detail = url.searchParams.get("error_description") ?? error; + return htmlResponse(errorPageHtml( + "Authorization failed", `${detail} Start the connection again.`), 400); + } + + const state = url.searchParams.get("state") ?? ""; + const separator = state.indexOf(":"); + const code = url.searchParams.get("code"); + if (separator < 0 || !code) return htmlResponse(INVALID_LINK_HTML, 400); + + let account: OAuthCallbackAccount; + try { + account = accountForId(state.slice(0, separator)); + } catch { + return htmlResponse(INVALID_LINK_HTML, 400); + } + + try { + const accepted = await account.acceptAuthCode( + code, state.slice(separator + 1), url.searchParams.get("iss") ?? undefined); + if (!accepted) return htmlResponse(INVALID_LINK_HTML, 400); + } catch (err) { + log.warn("oauth code exchange failed", { event: "connect.oauth.failed", error: err }); + return htmlResponse(errorPageHtml( + "Could not finish connecting", err instanceof Error ? err.message : String(err)), 502); + } + return htmlResponse(SELF_CLOSING_HTML); +} + /** Routes the HTTP paths common to both MCP connectors. */ export async function handleMcpHttpRequest( request: Request, diff --git a/packages/mcp-shared/src/oauth-callback.ts b/packages/mcp-shared/src/oauth-callback.ts deleted file mode 100644 index 031794361..000000000 --- a/packages/mcp-shared/src/oauth-callback.ts +++ /dev/null @@ -1,45 +0,0 @@ -import type { McpLog } from "./log.js"; -import { - errorPageHtml, htmlResponse, INVALID_LINK_HTML, SELF_CLOSING_HTML, -} from "./html.js"; - -type OAuthCallbackAccount = { - acceptAuthCode(code: string, nonce: string, issuer?: string): Promise; -}; - -/** Completes the shared browser callback for an MCP account OAuth flow. */ -export async function handleOAuthCallback( - url: URL, - accountForId: (id: string) => OAuthCallbackAccount, - log: McpLog, -): Promise { - const error = url.searchParams.get("error"); - if (error) { - const detail = url.searchParams.get("error_description") ?? error; - return htmlResponse(errorPageHtml( - "Authorization failed", `${detail} Start the connection again.`), 400); - } - - const state = url.searchParams.get("state") ?? ""; - const separator = state.indexOf(":"); - const code = url.searchParams.get("code"); - if (separator < 0 || !code) return htmlResponse(INVALID_LINK_HTML, 400); - - let account: OAuthCallbackAccount; - try { - account = accountForId(state.slice(0, separator)); - } catch { - return htmlResponse(INVALID_LINK_HTML, 400); - } - - try { - const accepted = await account.acceptAuthCode( - code, state.slice(separator + 1), url.searchParams.get("iss") ?? undefined); - if (!accepted) return htmlResponse(INVALID_LINK_HTML, 400); - } catch (err) { - log.warn("oauth code exchange failed", { event: "connect.oauth.failed", error: err }); - return htmlResponse(errorPageHtml( - "Could not finish connecting", err instanceof Error ? err.message : String(err)), 502); - } - return htmlResponse(SELF_CLOSING_HTML); -} diff --git a/packages/mcp-shared/src/portal.ts b/packages/mcp-shared/src/portal.ts index 74679b859..d34434e43 100644 --- a/packages/mcp-shared/src/portal.ts +++ b/packages/mcp-shared/src/portal.ts @@ -20,6 +20,14 @@ import type { McpTool } from "./client.js"; */ export const PORTAL_LIST_SERVERS_TOOL = "portal_list_servers"; +/** + * The portal's tool for opening a URL-based re-authentication page. This is the only recovery path + * Cloudflare's MCP Server Portals expose when an upstream server's on-behalf OAuth has lapsed -- + * there is no dashboard alternative. See `parsePortalReconnectUrl` and the MCP Server Portals + * connector's README. + */ +export const PORTAL_TOGGLE_SERVERS_TOOL = "portal_toggle_servers"; + // Prefix the portal reserves for its own session-management tools. const PORTAL_NATIVE_PREFIX = "portal_"; @@ -222,6 +230,29 @@ export function parsePortalServers( return structured ?? { servers: [], complete: false }; } +// Matches the first `https://` URL in prose, stopping at whitespace or a character that commonly +// closes a URL when one is embedded in a sentence (closing paren/bracket or a quote). +const HTTPS_URL_PATTERN = /https:\/\/[^\s<>")]+/; + +/** + * Recovers the re-authentication URL from a `portal_toggle_servers` result, or null if none was + * found. + * + * `portal_toggle_servers`'s response shape is undocumented beyond "opens a URL-based server + * selection page", so this reads prose the same way `parsePortalServers` does rather than assuming + * a structured field exists. Returning null instead of guessing lets the caller log the full + * response for later refinement rather than surfacing an unverified shape to the user. + */ +export function parsePortalReconnectUrl(result: { content?: unknown }): string | null { + const combinedText = (Array.isArray(result.content) ? result.content : []) + .flatMap(block => { + const { type, text: blockText } = (block ?? {}) as { type?: unknown; text?: unknown }; + return type === "text" && typeof blockText === "string" ? [blockText] : []; + }) + .join("\n"); + return combinedText.match(HTTPS_URL_PATTERN)?.[0] ?? null; +} + /** * Merges the portal's reported servers with the ids present in a complete tool index. Tool names * are the authority, so reported empty servers are dropped. diff --git a/packages/workshop-backend/__integration__/open-gadget-rpc.test.ts b/packages/workshop-backend/__integration__/open-gadget-rpc.test.ts index 2609d03a8..8aa29ecea 100644 --- a/packages/workshop-backend/__integration__/open-gadget-rpc.test.ts +++ b/packages/workshop-backend/__integration__/open-gadget-rpc.test.ts @@ -139,7 +139,7 @@ describe.skip("openGadget errors across native RPC and Cap'n Web", () => { }); // In production, workerd tags rejections from a reset DO with the structured flags -// do-telemetry.ts reads. Locally, vitest-pool-workers aborts reject FLAGLESS — this test pins that, so if a +// do-retry.ts reads. Locally, vitest-pool-workers aborts reject FLAGLESS — this test pins that, so if a // future pool upgrade starts attaching the production flags, it fails and the flag paths can // graduate from synthetic unit tests to real-reset integration tests. abortAllDurableObjects() // is the non-graceful teardown (deliberately not evictDurableObject(), which never breaks a diff --git a/packages/workshop-backend/__tests__/do-retry.test.ts b/packages/workshop-backend/__tests__/do-retry.test.ts new file mode 100644 index 000000000..369e00bbd --- /dev/null +++ b/packages/workshop-backend/__tests__/do-retry.test.ts @@ -0,0 +1,169 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { isDoResetError, retryOnDoReset } from "../src/do-retry"; +import { createWorkshopLogger } from "../src/observability"; + +// Synthetic errors shaped like workerd's tagged rejections (jsg/util.c++). Local aborts reject +// flagless (pinned by the "user-DO reset flags" integration test), so the predicates and the +// retry path are exercised here with the production shapes. +function resetError(flags: Record): Error { + return Object.assign(new Error("Durable Object reset."), flags); +} + +// The shape a production storage-timeout reset arrives in: a dead incarnation, flagged overloaded. +const PRODUCTION_RESET = { remote: true, overloaded: true, durableObjectReset: true }; + +describe("isDoResetError", () => { + it("matches the durableObjectReset flag", () => { + expect(isDoResetError(resetError({ durableObjectReset: true }))).toBe(true); + }); + + it("matches the retryable flag (connection lost)", () => { + expect(isDoResetError(resetError({ retryable: true }))).toBe(true); + }); + + it("matches the production storage-timeout shape (overloaded reset)", () => { + expect(isDoResetError(resetError(PRODUCTION_RESET))).toBe(true); + }); + + it("rejects overload without a reset (live object shedding load)", () => { + expect(isDoResetError(resetError({ remote: true, overloaded: true }))).toBe(false); + }); + + it("rejects unflagged and malformed values", () => { + expect(isDoResetError(new Error("some app error"))).toBe(false); + expect(isDoResetError(resetError({ durableObjectReset: "yes" }))).toBe(false); + expect(isDoResetError(resetError({ retryable: 1 }))).toBe(false); + expect(isDoResetError(null)).toBe(false); + expect(isDoResetError(undefined)).toBe(false); + expect(isDoResetError("boom")).toBe(false); + }); +}); + +// --------------------------------------------------------------------------------------- +// retryOnDoReset. Thunks are the whole harness: counting invocations asserts the retry (and +// single-retry) behavior, and identity checks on rejections pin the flag contract the frontend +// classifier (workshop-frontend's rpcErrors.ts) depends on. The logger writes through console, +// so `user_do.reset.recovered` is observed by spying on console.info. + +function failingThunk(errors: unknown[], value = "ok") { + let calls = 0; + const call = () => { + const error = errors[calls++]; + return error === undefined ? Promise.resolve(value) : Promise.reject(error); + }; + return { call, count: () => calls }; +} + +function recoveredEvents(spy: ReturnType): number { + return spy.mock.calls.filter( + ([entry]) => (entry as { event?: unknown })?.event === "user_do.reset.recovered").length; +} + +function spies() { + vi.spyOn(Math, "random").mockReturnValue(0); // pin the jitter to a zero wait + return vi.spyOn(console, "info").mockImplementation(() => {}); +} + +describe("retryOnDoReset", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("passes a resolving call through: one invocation, no log", async () => { + const info = spies(); + const thunk = failingThunk([]); + + expect(await retryOnDoReset(thunk.call)).toBe("ok"); + expect(thunk.count()).toBe(1); + expect(recoveredEvents(info)).toBe(0); + }); + + it("recovers from the production reset shape: two invocations, one recovery log", async () => { + const info = spies(); + const thunk = failingThunk([resetError(PRODUCTION_RESET)]); + + expect(await retryOnDoReset(thunk.call)).toBe("ok"); + expect(thunk.count()).toBe(2); + expect(recoveredEvents(info)).toBe(1); + }); + + it("attributes the recovery to the caller's logger when one is passed", async () => { + const info = spies(); + const thunk = failingThunk([resetError(PRODUCTION_RESET)]); + + const log = createWorkshopLogger("workshop.overseer").with({ gadgetId: "g1" }); + expect(await retryOnDoReset(thunk.call, log)).toBe("ok"); + const recovered = info.mock.calls.map(([entry]) => entry as Record) + .filter(entry => entry.event === "user_do.reset.recovered"); + expect(recovered).toEqual( + [expect.objectContaining({ component: "workshop.overseer", gadgetId: "g1" })]); + }); + + it("waits a jittered delay bounded by the retry window", async () => { + vi.spyOn(Math, "random").mockReturnValue(0.5); + vi.spyOn(console, "info").mockImplementation(() => {}); + const wait = vi.spyOn(scheduler, "wait").mockResolvedValue(undefined); + const thunk = failingThunk([resetError(PRODUCTION_RESET)]); + + expect(await retryOnDoReset(thunk.call)).toBe("ok"); + expect(wait).toHaveBeenCalledExactlyOnceWith(0.5 * 250); // Math.random() * RETRY_JITTER_MS + }); + + it("retries a bare retryable rejection (connection lost)", async () => { + const info = spies(); + const thunk = failingThunk([resetError({ retryable: true })]); + + expect(await retryOnDoReset(thunk.call)).toBe("ok"); + expect(thunk.count()).toBe(2); + expect(recoveredEvents(info)).toBe(1); + }); + + it("does not retry retryable+overloaded (live object shedding load)", async () => { + const info = spies(); + const error = resetError({ retryable: true, overloaded: true }); + const thunk = failingThunk([error]); + + await expect(retryOnDoReset(thunk.call)).rejects.toBe(error); // identity, flags intact + expect(thunk.count()).toBe(1); + expect(recoveredEvents(info)).toBe(0); + }); + + it("does not retry a flagless error (the local-abort shape)", async () => { + const info = spies(); + const error = new Error("Durable Object reset."); + const thunk = failingThunk([error]); + + await expect(retryOnDoReset(thunk.call)).rejects.toBe(error); + expect(thunk.count()).toBe(1); + expect(recoveredEvents(info)).toBe(0); + }); + + it("does not retry an app error", async () => { + const info = spies(); + const error = new Error("some app error"); + const thunk = failingThunk([error]); + + await expect(retryOnDoReset(thunk.call)).rejects.toBe(error); + expect(thunk.count()).toBe(1); + expect(recoveredEvents(info)).toBe(0); + }); + + it("retries exactly once: a second rejection propagates by identity, flags intact", async () => { + const info = spies(); + const first = resetError(PRODUCTION_RESET); + const second = resetError(PRODUCTION_RESET); + const thunk = failingThunk([first, second]); + + let caught: unknown; + try { + await retryOnDoReset(thunk.call); + } catch (e) { + caught = e; + } + expect(caught).toBe(second); // the retry's own rejection, not a re-wrap + expect(thunk.count()).toBe(2); // single retry by construction + // The frontend classifier reads the flags as own enumerable props; pin that they survive. + expect({ ...(caught as object) }).toMatchObject(PRODUCTION_RESET); + expect(recoveredEvents(info)).toBe(0); + }); +}); diff --git a/packages/workshop-backend/__tests__/do-telemetry.test.ts b/packages/workshop-backend/__tests__/do-telemetry.test.ts deleted file mode 100644 index 9f1df4bfe..000000000 --- a/packages/workshop-backend/__tests__/do-telemetry.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { isDoResetError } from "../src/do-telemetry"; - -// Synthetic errors shaped like workerd's tagged rejections (jsg/util.c++). Local aborts reject -// flagless (pinned by the "user-DO reset flags" integration test), so the predicate is -// exercised here with the production shapes. -function resetError(flags: Record): Error { - return Object.assign(new Error("Durable Object reset."), flags); -} - -describe("isDoResetError", () => { - it("matches the durableObjectReset flag", () => { - expect(isDoResetError(resetError({ durableObjectReset: true }))).toBe(true); - }); - - it("matches the retryable flag (connection lost)", () => { - expect(isDoResetError(resetError({ retryable: true }))).toBe(true); - }); - - it("matches the production storage-timeout shape (overloaded reset)", () => { - expect(isDoResetError( - resetError({ remote: true, overloaded: true, durableObjectReset: true }))).toBe(true); - }); - - it("rejects overload without a reset (live object shedding load)", () => { - expect(isDoResetError(resetError({ remote: true, overloaded: true }))).toBe(false); - }); - - it("rejects unflagged and malformed values", () => { - expect(isDoResetError(new Error("some app error"))).toBe(false); - expect(isDoResetError(resetError({ durableObjectReset: "yes" }))).toBe(false); - expect(isDoResetError(resetError({ retryable: 1 }))).toBe(false); - expect(isDoResetError(null)).toBe(false); - expect(isDoResetError(undefined)).toBe(false); - expect(isDoResetError("boom")).toBe(false); - }); -}); diff --git a/packages/workshop-backend/src/do-retry.ts b/packages/workshop-backend/src/do-retry.ts new file mode 100644 index 000000000..90570aaff --- /dev/null +++ b/packages/workshop-backend/src/do-retry.ts @@ -0,0 +1,110 @@ +// Retry and telemetry for Durable Object reset rejections. +// +// workerd attaches the flags natively in the calling Worker, so no message matching is needed +// (jsg/util.c++, decodeTunneledException). Two independent axes: `retryable`/`overloaded` come +// from the kj exception TYPE (DISCONNECTED/OVERLOADED) and describe THIS CALL — one type per +// hop, so one flag per hop; `durableObjectReset` is parsed from the tunneled description and +// describes THE OBJECT, whose incarnation died, poisoning every stub to it. Hence the production +// storage-timeout reset is `{remote, overloaded, durableObjectReset}` with no `retryable`: it was +// shedding load AND it died. The docs cover `retryable`/`overloaded`/`remote`, but not +// `durableObjectReset` or the `durableObjectId` we log: +// https://developers.cloudflare.com/durable-objects/best-practices/error-handling/ +// +// Local vitest-pool-workers aborts reject FLAGLESS (pinned by the "user-DO reset flags" +// integration test), so the predicates and the retry path are unit-tested with synthetic +// production shapes. + +import { createWorkshopLogger } from "./observability"; + +const logger = createWorkshopLogger("workshop.server"); + +/** + * True for rejections caused by a DO reset or lost connection. This is the telemetry + * classification, deliberately not the retry policy, which is narrower (see + * `shouldRetryAfterReset`): a bare `overloaded` is excluded here only because a live object + * shedding load is not a reset. + */ +export function isDoResetError(e: unknown): boolean { + if (typeof e !== "object" || e === null) return false; + const flags = e as { durableObjectReset?: unknown; retryable?: unknown }; + return flags.durableObjectReset === true || flags.retryable === true; +} + +/** Wraps a DO stub so every method call observes DO-reset rejections for telemetry + * (`user_do.reset.surfaced`, with the method name as the operation) and rethrows them + * unchanged. Otherwise transparent. Pass the caller's logger so the log attributes the reset + * to the component (and context, e.g. gadgetId) that observed it; defaults to the Worker's. + * A surfaced reset may still be absorbed by `retryOnDoReset` at the call site (correlate with + * `user_do.reset.recovered`). */ +export function wrapDoStubForTelemetry( + stub: T, log: ReturnType = logger): T { + return new Proxy(stub, { + get(target, prop) { + const value = Reflect.get(target, prop) as unknown; + if (typeof value !== "function") return value; + // Invoke through the stub (`target[prop](...)`) rather than `.apply` on the extracted + // handle: native RPC method handles are themselves proxies, and touching `.apply` on one + // is interpreted as a nested RPC property access (the DO then rejects a call to "apply"). + const methods = target as unknown as Record unknown>; + if (typeof prop !== "string") return (...args: unknown[]) => methods[prop](...args); + return (...args: unknown[]) => { + const result = methods[prop](...args); + if (typeof (result as PromiseLike | undefined)?.then !== "function") return result; + return (async () => { + try { + return await (result as PromiseLike); + } catch (e) { + if (isDoResetError(e)) { + log.warn("user DO reset observed", { + event: "user_do.reset.surfaced", + operation: prop, + durableObjectId: target.id.toString(), + error: e, + }); + } + throw e; + } + })(); + }; + }, + }); +} + +// Full jitter decorrelates the replay burst a mass reset produces (a reset fails every +// in-flight call from every session at once); workerd queues the fresh-stub request behind +// the object restart, so no delay floor is needed. +const RETRY_JITTER_MS = 250; + +/** Whether a rejection may be retried, given a call already known to be replay-safe. + * Narrower than `isDoResetError`: `durableObjectReset` retries even with `overloaded` set (the + * incarnation is dead — the queue that overloaded it died with it; this is the shape production + * storage-timeout resets arrive in), a deliberate divergence from the never-retry-`overloaded` + * guidance in the error-handling docs linked above. Bare `retryable` (connection lost to a + * possibly-live object) retries only if it isn't shedding load. */ +function shouldRetryAfterReset(e: unknown): boolean { + if (typeof e !== "object" || e === null) return false; + const flags = e as { durableObjectReset?: unknown; retryable?: unknown; overloaded?: unknown }; + if (flags.durableObjectReset === true) return true; + return flags.retryable === true && flags.overloaded !== true; +} + +/** Retries `callWithFreshStub` once if it rejects with a DO-reset shape. The caller asserts the + * call is replay-safe (a reset can't distinguish "never applied" from "applied, response lost") + * — wrap pure reads only. The thunk must mint its stub inside itself (e.g. via the fresh-stub + * getters) so the second attempt gets a fresh incarnation; a captured stub is permanently broken + * and retrying it is a silent no-op. Pass the same logger the stub's `wrapDoStubForTelemetry` + * uses so the recovery is attributed to the component (and context, e.g. gadgetId) whose + * surfaced warning it absorbs; defaults to the Worker's. */ +export async function retryOnDoReset( + callWithFreshStub: () => Promise, + log: ReturnType = logger): Promise { + try { + return await callWithFreshStub(); + } catch (e) { + if (!shouldRetryAfterReset(e)) throw e; // identity rethrow, flags intact + await scheduler.wait(Math.random() * RETRY_JITTER_MS); + let recovered = await callWithFreshStub(); // a second rejection propagates by identity + log.info("recovered from user DO reset", { event: "user_do.reset.recovered" }); + return recovered; + } +} diff --git a/packages/workshop-backend/src/do-telemetry.ts b/packages/workshop-backend/src/do-telemetry.ts deleted file mode 100644 index 7b49a0007..000000000 --- a/packages/workshop-backend/src/do-telemetry.ts +++ /dev/null @@ -1,62 +0,0 @@ -// Telemetry for Durable Object reset rejections. -// -// workerd tags rejections from a reset or disconnected DO with structured flags (jsg/util.c++): -// `retryable` ⇔ connection lost, `overloaded` ⇔ load shedding, and `durableObjectReset` -// whenever the object's incarnation died — the production storage-timeout reset arrives as -// `{remote, overloaded, durableObjectReset}`. The flags are attached natively in the calling -// Worker, so no message matching is needed. Local vitest-pool-workers aborts reject FLAGLESS -// (pinned by the "user-DO reset flags" integration test), so this predicate is unit-tested -// with synthetic production shapes. - -import { createWorkshopLogger } from "./observability"; - -const logger = createWorkshopLogger("workshop.server"); - -/** - * True for rejections caused by a DO reset or lost connection. These are requests that could make - * sense to retry (although as of this writing, the code does not do so). `overloaded` is excluded - * because when the DO is overloaded, retrying would make the problem worse. - */ -export function isDoResetError(e: unknown): boolean { - if (typeof e !== "object" || e === null) return false; - const flags = e as { durableObjectReset?: unknown; retryable?: unknown }; - return flags.durableObjectReset === true || flags.retryable === true; -} - -/** Wraps a DO stub so every method call observes DO-reset rejections for telemetry - * (`user_do.reset.surfaced`, with the method name as the operation) and rethrows them - * unchanged. Otherwise transparent. Pass the caller's logger so the log attributes the reset - * to the component (and context, e.g. gadgetId) that observed it; defaults to the Worker's. */ -export function wrapDoStubForTelemetry( - stub: T, log: ReturnType = logger): T { - return new Proxy(stub, { - get(target, prop) { - const value = Reflect.get(target, prop) as unknown; - if (typeof value !== "function") return value; - // Invoke through the stub (`target[prop](...)`) rather than `.apply` on the extracted - // handle: native RPC method handles are themselves proxies, and touching `.apply` on one - // is interpreted as a nested RPC property access (the DO then rejects a call to "apply"). - const methods = target as unknown as Record unknown>; - if (typeof prop !== "string") return (...args: unknown[]) => methods[prop](...args); - return (...args: unknown[]) => { - const result = methods[prop](...args); - if (typeof (result as PromiseLike | undefined)?.then !== "function") return result; - return (async () => { - try { - return await (result as PromiseLike); - } catch (e) { - if (isDoResetError(e)) { - log.warn("user DO reset observed", { - event: "user_do.reset.surfaced", - operation: prop, - durableObjectId: target.id.toString(), - error: e, - }); - } - throw e; - } - })(); - }; - }, - }); -} diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 04695021a..909ae7074 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -38,7 +38,7 @@ import { SharingManager, SharingCaller, CollaboratorRecord, ShareKeyRecord } fro import { AutoApprovalDrainer } from "./auto-approval"; import { collectSlashCommands, invokeSlashCommand } from "./slash-commands"; import { createWorkshopLogger, obsContext, traced } from "./observability"; -import { wrapDoStubForTelemetry } from "./do-telemetry"; +import { retryOnDoReset, wrapDoStubForTelemetry } from "./do-retry"; import type { ChatGatewayRpcTarget, SubmitExternalMessageResult } from "@gadgets/workshop-shared/external-message-gateway"; import type { GadgetExportFormat } from "@gadgets/workshop-shared/api"; import { @@ -4512,7 +4512,8 @@ class OverseerImpl implements AgentHooks { #ownerUserDo() { if (!this.ownerId) throw new Error("Workspace is not initialized."); - return this.users.get(this.users.idFromString(this.ownerId)); + return wrapDoStubForTelemetry( + this.users.get(this.users.idFromString(this.ownerId)), this.logger); } // Ensure every singleton account the gadget owner has (e.g. the Context Library) is provisioned @@ -4710,7 +4711,9 @@ class OverseerImpl implements AgentHooks { : Promise<{config: AiModelConfig, initiator: AiChatAuthorInfo} | undefined> { if (!this.ownerId) return undefined; try { - let userMeta = await this.#ownerUserDo().getChatContext(null); + // Pure read on a fresh-stub getter: safe to retry once across a user-DO reset. + let userMeta = await retryOnDoReset( + () => this.#ownerUserDo().getChatContext(null), this.logger); return userMeta.quickModel ? {config: userMeta.quickModel, initiator: userMeta.profile} : undefined; @@ -5699,7 +5702,8 @@ class OverseerImpl implements AgentHooks { #ownerUserStub() { if (!this.ownerId) throw new Error("Workspace has been deleted."); - return this.users.get(this.users.idFromString(this.ownerId)); + return wrapDoStubForTelemetry( + this.users.get(this.users.idFromString(this.ownerId)), this.logger); } // Short-TTL cache for the gatekeeper vendor list. The list is derived from static @@ -5716,7 +5720,8 @@ class OverseerImpl implements AgentHooks { if (this.#vendorsCache && this.#vendorsCache.expires > now) { return this.#vendorsCache.promise; } - let promise = this.#ownerUserStub().listGatekeeperVendors(); + let promise = retryOnDoReset( + () => this.#ownerUserStub().listGatekeeperVendors(), this.logger); // Don't cache failures: drop the entry so the next call retries. promise.catch(() => { if (this.#vendorsCache?.promise === promise) this.#vendorsCache = null; @@ -7421,10 +7426,12 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async #getClientProfile(): Promise { if (!this.#clientProfilePromise) { - this.#clientProfilePromise = this.#clientUser.whoami().catch((err: unknown) => { - this.#clientProfilePromise = undefined; - throw err; - }); + this.#clientProfilePromise = retryOnDoReset( + () => this.#clientUser.whoami(), this.impl.logger) + .catch((err: unknown) => { + this.#clientProfilePromise = undefined; + throw err; + }); } const profilePromise = this.#clientProfilePromise!; @@ -7441,7 +7448,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { defaultGadgetId: this.impl.defaultGadgetId, }; if (!this.isOwner) { - result.owner = await this.#owner.whoami(); + result.owner = await retryOnDoReset(() => this.#owner.whoami(), this.impl.logger); } return result; } @@ -7462,7 +7469,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // For collaborators, include owner info. if (!this.isOwner) { - metadata.owner = await this.#owner.whoami(); + metadata.owner = await retryOnDoReset(() => this.#owner.whoami(), this.impl.logger); } let titleSubscriber = { @@ -7543,7 +7550,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { let taken = new Set( [...this.impl.storage.gadgets.list()].map(gadget => gadget.bindingName)); for (let name of chatNames ?? []) taken.add(name); - let userMeta = await this.#clientUser.getChatContext(null); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(null), this.impl.logger); if (userMeta.quickModel) { bindingName = await this.impl.generateBindingName( title, taken, {config: userMeta.quickModel, initiator: userMeta.profile}); @@ -7745,7 +7753,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } async newAiModelGatekeeper(modelId: string): Promise> { - let chatMeta = await this.#clientUser.getChatContext(modelId); + let chatMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(modelId), this.impl.logger); let props: LanguageModelGatekeeperProps = { displayName: chatMeta.aiModel!.profile.name, config: chatMeta.aiModel!.config, @@ -7801,7 +7810,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { config, }; if (config.modelId) { - let chatMeta = await this.#clientUser.getChatContext(config.modelId); + let chatMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(config.modelId), this.impl.logger); if (chatMeta.aiModel) { creationSpec.modelProvider = chatMeta.aiModel.config.provider; creationSpec.modelName = chatMeta.aiModel.config.model; @@ -8109,7 +8119,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } } - let userMeta = await this.#clientUser.getChatContext(modelId); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(modelId), this.impl.logger); if (!userMeta.aiModel) return; // No model resolved; nothing to resume. let preparation = this.impl.waitForChatMessagePreparation(chatId); @@ -8226,7 +8237,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } async listModels(): Promise { - return this.#clientUser.listModels(); + return retryOnDoReset(() => this.#clientUser.listModels(), this.impl.logger); } async listSlashCommands(): Promise { @@ -8240,7 +8251,9 @@ class OverseerClientInterface extends RpcTarget implements Overseer { ): Promise { let provider: AiModelConfig["provider"] | undefined; if (modelId !== null) { - provider = (await this.#clientUser.getChatContext(modelId)).aiModel?.config.provider; + provider = (await retryOnDoReset( + () => this.#clientUser.getChatContext(modelId), this.impl.logger)) + .aiModel?.config.provider; } attachment = validateChatAttachmentUpload( attachment, @@ -8464,7 +8477,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async newChat(initialMessage: string | SlashCommandRequest, chosenModelId: string | null, capsules?: CapsuleSpecifier[], attachments?: ChatAttachmentHandle[], formats?: MessageFormatRef[]): Promise { - let userMeta = await this.#clientUser.getChatContext(chosenModelId); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(chosenModelId), this.impl.logger); return this.impl.newChat(this.#clientUser, userMeta, initialMessage, capsules, attachments, undefined, undefined, formats); } @@ -8473,7 +8487,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { chatId: number, message: string | SlashCommandRequest, chosenModelId: string | null, capsules?: CapsuleSpecifier[], attachments?: ChatAttachmentHandle[], formats?: MessageFormatRef[]): Promise { - let userMeta = await this.#clientUser.getChatContext(chosenModelId); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(chosenModelId), this.impl.logger); return this.impl.sendChatMessage( this.#clientUser, userMeta, chatId, message, capsules, attachments, undefined, formats); } @@ -8490,7 +8505,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async mergeChanges(chatId: number, mergeThrough: number | null, options?: { includeDraft?: boolean }): Promise { - let userMeta = await this.#clientUser.getChatContext(null); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(null), this.impl.logger); let meta = this.impl.assertChatNotActive(chatId); if (options?.includeDraft) { @@ -8756,7 +8772,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } async retryAgent(chatId: number, modelId: string): Promise { - let userMeta = await this.#clientUser.getChatContext(modelId); + let userMeta = await retryOnDoReset( + () => this.#clientUser.getChatContext(modelId), this.impl.logger); let meta = this.impl.assertChatNotActive(chatId); if (!userMeta.aiModel) { @@ -9009,7 +9026,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // Check if the creator is the owner (requires an RPC to the owner's DO). let ownerProfileId = await this.impl.getOwnerProfileId(); if (ownerProfileId === record.createdBy) { - createdBy = await this.#owner.whoami(); + createdBy = await retryOnDoReset(() => this.#owner.whoami(), this.impl.logger); } // Check if the creator is a collaborator (resolved locally). if (!createdBy) { @@ -9061,7 +9078,8 @@ class UseOverseerInterface extends RpcTarget implements Overseer { private notifyClosed: NativeRpcStub<() => void>) { super(); this.#leavePresence = joinSessionPresence( - this.impl, this.clientProfileId, "use", () => this.#clientUser.whoami()); + this.impl, this.clientProfileId, "use", + () => retryOnDoReset(() => this.#clientUser.whoami(), this.impl.logger)); this.#leaveOutputsFanout = this.impl.joinOutputsFanout(this.clientUserId); } @@ -9100,7 +9118,7 @@ class UseOverseerInterface extends RpcTarget implements Overseer { return { id: this.impl.ctx.id.toString(), title: this.impl.storage.title.get(), - owner: await this.#owner.whoami(), + owner: await retryOnDoReset(() => this.#owner.whoami(), this.impl.logger), role: "use", defaultGadgetId: this.impl.defaultGadgetId, }; @@ -9114,7 +9132,7 @@ class UseOverseerInterface extends RpcTarget implements Overseer { let metadata: GadgetMetadata = { id: this.impl.ctx.id.toString(), title: this.impl.storage.title.get(), - owner: await this.#owner.whoami(), + owner: await retryOnDoReset(() => this.#owner.whoami(), this.impl.logger), role: "use", defaultGadgetId: this.impl.defaultGadgetId, }; @@ -9382,7 +9400,7 @@ class GadgetClientImpl extends RpcTarget implements GadgetClient { if (!this.impl.storage.chatMeta.get(chatId)) { throw new Error(`No such chat: ${chatId}`); } - let author = await this.#clientUser.whoami(); + let author = await retryOnDoReset(() => this.#clientUser.whoami(), this.impl.logger); this.impl.bindWorkpiece(this.id, name, target, chatId); this.impl.addChatMessages(chatId, author, [{ type: "changes", diff --git a/packages/workshop-backend/src/server.ts b/packages/workshop-backend/src/server.ts index ebb47b8d6..cf7d19aeb 100644 --- a/packages/workshop-backend/src/server.ts +++ b/packages/workshop-backend/src/server.ts @@ -28,7 +28,7 @@ import { verifyCfAccessJwt } from "./access.js"; import { resolveUiFeatureFlags } from "./feature-flags"; import { serveSiteLogo, SITE_LOGO_PATH } from "./site-logo.js"; import { createWorkshopLogger } from "./observability"; -import { wrapDoStubForTelemetry } from "./do-telemetry"; +import { retryOnDoReset, wrapDoStubForTelemetry } from "./do-retry"; const logger = createWorkshopLogger("workshop.server"); @@ -117,7 +117,8 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } whoami(): Promise { - return this.#user.whoami(); + // Pure-read delegations retry once across a user-DO reset (see retryOnDoReset); writes never do. + return retryOnDoReset(() => this.#user.whoami()); } setOwnDisplayName(name: string): Promise { return this.#user.setOwnDisplayName(name); @@ -126,10 +127,10 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { return this.#user.changePassword(oldHash, newHash); } hasPasswordLogin(): Promise { - return this.#user.hasPasswordLogin(); + return retryOnDoReset(() => this.#user.hasPasswordLogin()); } listModels(): Promise { - return this.#user.listModels(); + return retryOnDoReset(() => this.#user.listModels()); } addModel(profile: AiChatAuthorInfo, config: AiModelConfig): Promise { return this.#user.addModel(profile, config); @@ -141,17 +142,17 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { return this.#user.setQuickModel(id); } getQuickModel(): Promise { - return this.#user.getQuickModel(); + return retryOnDoReset(() => this.#user.getQuickModel()); } getPreferredModel(): Promise { - return this.#user.getPreferredModel(); + return retryOnDoReset(() => this.#user.getPreferredModel()); } setPreferredModel(id: string | null): Promise { return this.#user.setPreferredModel(id); } isOnboardingCompleted(): Promise { - return this.#user.isOnboardingCompleted(); + return retryOnDoReset(() => this.#user.isOnboardingCompleted()); } completeOnboarding(): Promise { return this.#user.completeOnboarding(); @@ -296,7 +297,7 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } async listGadgets(): Promise { - return this.#user.listGadgets(); + return retryOnDoReset(() => this.#user.listGadgets()); } listOutputs(): Promise { @@ -310,7 +311,7 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } listGatekeeperVendors(filter?: GatekeeperVendorFilter): Promise { - return this.#user.listGatekeeperVendors(filter); + return retryOnDoReset(() => this.#user.listGatekeeperVendors(filter)); } connectAccount(vendorId: string, resourceUrlPatterns?: string[]): Promise<{url: string}> { @@ -322,7 +323,7 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } listAddableGatekeepers(): Promise { - return this.#user.listAddableGatekeepers(); + return retryOnDoReset(() => this.#user.listAddableGatekeepers()); } provisionAmbientAccount(vendorId: string): Promise { @@ -354,15 +355,15 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } async listOwnBlueprints(): Promise { - return this.#user.listBlueprints(); + return retryOnDoReset(() => this.#user.listBlueprints()); } async getOwnBlueprint(blueprintId: string): Promise { - return this.#user.getBlueprint(blueprintId); + return retryOnDoReset(() => this.#user.getBlueprint(blueprintId)); } async listLibraryBlueprints(): Promise { - return this.#user.listLibraryBlueprints(); + return retryOnDoReset(() => this.#user.listLibraryBlueprints()); } async setBlueprintPinned(blueprintId: string, pinned: boolean): Promise { @@ -370,7 +371,7 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } async isBlueprintPinned(blueprintId: string): Promise { - return this.#user.isBlueprintPinned(blueprintId); + return retryOnDoReset(() => this.#user.isBlueprintPinned(blueprintId)); } async listFeaturedBlueprints(): Promise { @@ -387,7 +388,7 @@ class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi { } isBlueprintInLibrary(blueprintId: string): Promise<{ uploaded: boolean } | null> { - return this.#user.isBlueprintInLibrary(blueprintId); + return retryOnDoReset(() => this.#user.isBlueprintInLibrary(blueprintId)); } async importBlueprint(archive: ReadableStream): Promise { diff --git a/packages/workshop-backend/src/user.ts b/packages/workshop-backend/src/user.ts index a31dd2720..5ad4a239f 100644 --- a/packages/workshop-backend/src/user.ts +++ b/packages/workshop-backend/src/user.ts @@ -691,7 +691,8 @@ export class UserDurableObject extends DurableObject { resetAt: nextUtcMidnightIso() }; } - /** DO NOT MAKE PUBLIC -- returns API keys. */ + /** DO NOT MAKE PUBLIC -- returns API keys. Pure read: call sites replay it across DO resets + * via retryOnDoReset, so it must stay free of writes and side effects. */ async getChatContext(modelId: string | null): Promise { let gwConfig = getAiGatewayConfig(this.env); diff --git a/packages/workshop-frontend/src/components/ConnectConnectorModal.test.tsx b/packages/workshop-frontend/src/components/ConnectConnectorModal.test.tsx new file mode 100644 index 000000000..72fcc14a4 --- /dev/null +++ b/packages/workshop-frontend/src/components/ConnectConnectorModal.test.tsx @@ -0,0 +1,109 @@ +// @vitest-environment jsdom +/* eslint-disable react/react-in-jsx-scope */ + +import { act, type ComponentProps, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { VendorDescription } from '@gadgets/workshop-shared/gatekeeper' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +vi.mock('@cloudflare/kumo', () => { + const Dialog = Object.assign( + ({ children }: { children: ReactNode }) =>
{children}
, + { + Root: ({ children }: { children: ReactNode }) => <>{children}, + Title: ({ children }: { children: ReactNode }) =>

{children}

, + Description: ({ children }: { children: ReactNode }) =>

{children}

, + Close: ({ render }: { render: (props: object) => ReactNode }) => render({}), + }, + ) + return { + Dialog, + Switch: (props: ComponentProps<'input'>) => , + } +}) + +vi.mock('./WorkshopControls', () => ({ + WorkshopButton: ({ children, ...props }: ComponentProps<'button'>) => ( + + ), + WorkshopIconButton: ({ children, ...props }: ComponentProps<'button'>) => ( + + ), +})) + +import ConnectConnectorModal from './ConnectConnectorModal' + +const VENDOR: VendorDescription = { + displayName: 'MCP Server Portals', + url: 'https://developers.cloudflare.com/cloudflare-one/access-controls/ai-controls/mcp-portals/', + color: '#f6821f', +} + +describe('ConnectConnectorModal manage mode Reconnect', () => { + let root: Root | undefined + let container: HTMLDivElement | undefined + + afterEach(() => { + act(() => root?.unmount()) + container?.remove() + vi.restoreAllMocks() + root = undefined + container = undefined + }) + + async function render(props: Partial> = {}) { + container = document.createElement('div') + document.body.append(container) + root = createRoot(container) + await act(async () => { + root!.render( + {}} + credentialsValid + {...props} + />, + ) + await Promise.resolve() + }) + return container + } + + function reconnectButton(rendered: HTMLDivElement) { + return Array.from(rendered.querySelectorAll('button')) + .find((button) => button.textContent?.includes('Reconnect')) + } + + it('hides Reconnect when credentials are valid and alwaysOfferReconnect is unset', async () => { + const rendered = await render() + expect(reconnectButton(rendered)).toBeUndefined() + }) + + it('shows Reconnect when credentials have expired, regardless of alwaysOfferReconnect', async () => { + const rendered = await render({ credentialsValid: false, onReconnect: vi.fn() }) + expect(reconnectButton(rendered)).toBeDefined() + }) + + it('shows Reconnect for a connector with alwaysOfferReconnect even while credentials are valid', async () => { + // Mirrors the MCP Server Portals connector: its own `credentialsValid` can't see the portal's + // separate on-behalf authorization lapsing, so it always offers Reconnect here. + const onReconnect = vi.fn() + const rendered = await render({ + credentialsValid: true, + alwaysOfferReconnect: true, + onReconnect, + }) + const button = reconnectButton(rendered) + expect(button).toBeDefined() + + await act(async () => { + button!.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(onReconnect).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/workshop-frontend/src/components/ConnectConnectorModal.tsx b/packages/workshop-frontend/src/components/ConnectConnectorModal.tsx index 9e0f9bbdd..f4edb8888 100644 --- a/packages/workshop-frontend/src/components/ConnectConnectorModal.tsx +++ b/packages/workshop-frontend/src/components/ConnectConnectorModal.tsx @@ -28,6 +28,13 @@ interface ConnectConnectorModalProps { credentialsValid?: boolean disconnecting?: boolean onDisconnect?: () => void + // Manage mode: offer Reconnect here even while `credentialsValid` is true. Set for the MCP Server + // Portals connector, whose on-behalf authorization to an upstream server can lapse without this + // gatekeeper's own `credentialsValid` ever turning false -- see MCP_PORTAL_VENDOR_ID in + // routes/gatekeepers.tsx. + alwaysOfferReconnect?: boolean + onReconnect?: () => void + reconnecting?: boolean grantedResourceUrlPatterns?: string[] // Manage mode: invoked to expand the grant to include the given resource `urlPattern`s. onEnsureResources?: (resourceUrlPatterns: string[]) => void @@ -50,6 +57,9 @@ export default function ConnectConnectorModal({ credentialsValid = true, disconnecting = false, onDisconnect, + alwaysOfferReconnect = false, + onReconnect, + reconnecting = false, grantedResourceUrlPatterns, onEnsureResources, ensuringResourceUrlPatterns = [], @@ -392,6 +402,15 @@ export default function ConnectConnectorModal({ ) : ( <> + {(alwaysOfferReconnect || !credentialsValid) && onReconnect && ( + + {reconnecting ? 'Opening...' : 'Reconnect'} + + )} ( diff --git a/packages/workshop-frontend/src/routes/gatekeepers.tsx b/packages/workshop-frontend/src/routes/gatekeepers.tsx index 5902f17ea..040b238ab 100644 --- a/packages/workshop-frontend/src/routes/gatekeepers.tsx +++ b/packages/workshop-frontend/src/routes/gatekeepers.tsx @@ -30,6 +30,14 @@ export const Route = createFileRoute('/gatekeepers')({ component: ConnectorsPage, }) +// The MCP Server Portals gatekeeper's vendor id (see VENDOR_ID in gatekeeper-mcp-portal/src/portal.ts). +// That connector's `credentialsValid` only reflects its own OAuth with Cloudflare Access -- it can't +// see the portal's separate on-behalf authorization to whichever upstream server it fronts, so that +// authorization can lapse while the account still reads as connected here. Always offering Reconnect +// for this vendor is the only way a user can reach the portal's own re-authentication (see +// GatekeeperUserImpl.reconnect() in gatekeeper-mcp-portal), since nothing else surfaces the lapse. +const MCP_PORTAL_VENDOR_ID = 'mcp_portal' + interface AccountEntry { id: number accountDescription: AccountDescription @@ -86,6 +94,10 @@ interface ConnectorCardProps { onClick: () => void onReconnect?: () => void reconnectBusy?: boolean + // Offer Reconnect even while `state === 'connected'`. Used for the MCP Server Portals connector, + // whose on-behalf authorization to an upstream server can lapse without this gatekeeper's own + // `credentialsValid` (and thus `state`) ever turning `expired` -- see MCP_PORTAL_VENDOR_ID below. + alwaysOfferReconnect?: boolean view?: 'grid' | 'list' } @@ -101,6 +113,7 @@ function ConnectorCard({ onClick, onReconnect, reconnectBusy = false, + alwaysOfferReconnect = false, view = 'grid', }: ConnectorCardProps) { const handleKeyDown = (event: React.KeyboardEvent) => { @@ -136,13 +149,16 @@ function ConnectorCard({ ) : null + const showReconnect = + Boolean(onReconnect) && (state === 'expired' || (state === 'connected' && alwaysOfferReconnect)) + const trailing = - state === 'expired' && onReconnect ? ( + showReconnect ? (