diff --git a/public/js/dodo-settings.js b/public/js/dodo-settings.js index 87fd43d..9f899c4 100644 --- a/public/js/dodo-settings.js +++ b/public/js/dodo-settings.js @@ -288,7 +288,15 @@ function renderIntegrations(){ } }); configMap.forEach(cfg=>{ - connected.push(renderIntegCard(cfg.name,cfg.url||"Custom integration",null,cfg)); + // refresh_token configs need different action affordances — no Test + // button until /test understands them (it does, post-audit), AND a + // "Refresh token" button so users don't have to wait for the + // reconnect-once-on-401 path to fire. See renderRefreshTokenCard. + if(cfg.auth_type==="refresh_token"){ + connected.push(renderRefreshTokenCard(cfg)); + }else{ + connected.push(renderIntegCard(cfg.name,cfg.url||"Custom integration",null,cfg)); + } }); const cards=[...connected,...suggestions]; $("integrations-list").innerHTML=cards.length?cards.join(""):'
No integrations
'; @@ -310,6 +318,34 @@ function renderIntegCard(name,description,catalogUrl,config){ return `
${esc(name)}
${esc(description)}
${statusHtml}
${actionsHtml}
`; } +// refresh_token-MCP integrations (provisioned via set_refresh_token_mcp or +// start_dcr_oauth_flow). Distinct from the SDK-managed OAuth path: the +// access token lives in encrypted_secrets and Dodo refreshes it itself via +// the OAuth token endpoint. Actions: +// - Enable toggle (just flips the row's `enabled` flag — safe) +// - Test (now uses the bearer-aware /test endpoint; works post-audit) +// - Refresh token (force-refresh via /api/mcp-configs/:id/refresh-token, +// useful when the cached token is dead for reasons other than expiry) +// - Delete (wipes config + all encrypted secrets for this MCP) +function renderRefreshTokenCard(config){ + const checked=config.enabled?"checked":""; + const desc=config.url||"Refresh-token MCP integration"; + return `
${esc(config.name)}
${esc(desc)}
OAuth · auto-refresh
`; +} + +async function refreshIntegrationToken(id,displayName){ + try{ + const result=await json(`/api/mcp-configs/${encodeURIComponent(id)}/refresh-token`,{}); + if(result.accessToken){ + toast(`${displayName} token refreshed`,"success"); + }else if(result.error){ + toast(`Refresh failed: ${result.error}`,"error"); + }else{ + toast(`Refresh failed: unexpected response`,"error"); + } + }catch(e){toast("Refresh failed: "+(e.message||e),"error")} +} + // OAuth-MCP catalog entries render with different actions: "Connect with OAuth" // when not yet connected (opens the provider in a popup), and "Disconnect" // when already connected. The actual OAuth state is managed by the Agents SDK @@ -348,10 +384,11 @@ async function connectOAuthCatalog(catalogId,mcpUrl){ return; } // Poll for popup close + server-side connection — the Agents SDK - // handles the callback at /agents/oauth/callback and updates the - // hub DO's `getMcpServers()` state. We can't postMessage from the - // OAuth provider's domain, so we poll the popup's closed state and - // then refresh the integrations list. + // handles the callback at /agents/coding-agent//callback + // (see api/mcp/start-auth in src/index.ts for why the path is shaped + // this way) and updates the hub DO's `getMcpServers()` state. We + // can't postMessage from the OAuth provider's domain, so we poll + // the popup's closed state and then refresh the integrations list. const startedAt=Date.now(); const poll=setInterval(async()=>{ try{ diff --git a/src/index.ts b/src/index.ts index e0176c9..621a6fa 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1502,6 +1502,20 @@ app.post("/api/mcp-configs/:id/test", async (c) => { return proxyToUserControl(c.env, email, `/mcp-configs/${encodeURIComponent(c.req.param("id"))}/test`, { method: "POST" }); }); +// Force-refresh the OAuth access token for a refresh_token MCP config. +// Useful when the cached token has expired in some other way (revoked +// server-side, clock drift) and the user wants to retry without waiting +// for the next 401. UI surfaces this as a "Refresh token" button on +// refresh_token integrations. +app.post("/api/mcp-configs/:id/refresh-token", async (c) => { + const email = c.get("userEmail"); + return proxyToUserControl( + c.env, + email, + `/mcp-configs/${encodeURIComponent(c.req.param("id"))}/access-token?force=1`, + ); +}); + // OAuth success redirect — shown to the user after MCP OAuth completes. // The Agents SDK redirects here once token exchange finishes. Points the // browser back to the app root with a query flag the UI can react to. diff --git a/src/user-control.ts b/src/user-control.ts index e16174f..74c0e87 100644 --- a/src/user-control.ts +++ b/src/user-control.ts @@ -119,7 +119,11 @@ const mcpConfigUpdateSchema = z .object({ name: z.string().min(1).optional(), type: z.enum(["http", "service-binding"]).optional(), - auth_type: z.enum(["oauth", "static_headers"]).optional(), + // Accept refresh_token so a client that round-trips an existing config + // (GET → mutate enabled → PUT) doesn't get rejected. The handler still + // refuses to mutate refresh_token rows beyond the enabled flag — see + // updateMcpConfigEncrypted for the enforcement. + auth_type: z.enum(["oauth", "static_headers", "refresh_token"]).optional(), url: z.string().url().optional(), headers: z.record(z.string(), z.string()).optional(), enabled: z.boolean().optional(), @@ -922,10 +926,30 @@ export class UserControl extends DurableObject { if (request.method === "POST" && url.pathname.match(/^\/mcp-configs\/[^/]+\/test$/)) { const id = decodeURIComponent(url.pathname.split("/").at(-2) ?? ""); - const row = (Array.from(this.ctx.storage.sql.exec("SELECT id, name, type, url, headers_json, enabled FROM mcp_configs WHERE id = ?", id))[0] as SqlRow | null); + // Select auth_type so the mapper can distinguish refresh_token configs + // — without it the row was downgraded to static_headers and we never + // injected the bearer, so every refresh_token Test click returned 401. + const row = (Array.from(this.ctx.storage.sql.exec("SELECT id, name, type, auth_type, url, headers_json, enabled FROM mcp_configs WHERE id = ?", id))[0] as SqlRow | null); if (!row) return Response.json({ error: `MCP config ${id} not found` }, { status: 404 }); const ownerEmail = request.headers.get("x-owner-email") ?? ""; - const config = await this.resolveMcpConfigHeaders(this.mapMcpConfigRow(row), ownerEmail); + let config = this.mapMcpConfigRow(row); + + if (config.auth_type === "refresh_token") { + // refresh_token configs don't have headerKeys — they get their + // Authorization from the encrypted access_token (which is auto- + // refreshed by getMcpAccessToken if it's stale). + const accessToken = await this.getMcpAccessToken(id, ownerEmail); + if (!accessToken) { + return Response.json( + { ok: false, error: "No access token available — run set_refresh_token_mcp or start_dcr_oauth_flow to provision tokens" }, + { status: 200 }, + ); + } + config = { ...config, headers: { Authorization: `Bearer ${accessToken}` } }; + } else { + config = await this.resolveMcpConfigHeaders(config, ownerEmail); + } + const gatekeeper = new HttpMcpClient(config); const result = await gatekeeper.testConnection(); return Response.json(result); @@ -2257,11 +2281,54 @@ export class UserControl extends DurableObject { /** * Update an MCP config, replacing encrypted header secrets if new headers provided. + * + * For `refresh_token` configs only the `enabled` flag may be flipped via + * this path. Mutating `headers`, `url`, `auth_type`, or `type` would + * corrupt the token chain (see Hole 2 in the PR #91 audit). Use + * `set_refresh_token_mcp` / `start_dcr_oauth_flow` to re-provision a + * refresh_token config instead of trying to PATCH it. */ - private async updateMcpConfigEncrypted(id: string, patch: { name?: string; type?: string; auth_type?: "oauth" | "static_headers"; url?: string; headers?: Record; enabled?: boolean }, ownerEmail: string): Promise { + private async updateMcpConfigEncrypted(id: string, patch: { name?: string; type?: string; auth_type?: "oauth" | "static_headers" | "refresh_token"; url?: string; headers?: Record; enabled?: boolean }, ownerEmail: string): Promise { const current = this.getMcpConfigSafe(id); const now = nowEpoch(); + if (current.auth_type === "refresh_token") { + // Forbid every mutation that would break the OAuth token chain. The + // `enabled` toggle is safe because it doesn't touch encrypted_secrets + // or any of the oauth_* columns. + const forbidden: string[] = []; + if (patch.headers) forbidden.push("headers"); + if (patch.url && patch.url !== current.url) forbidden.push("url"); + if (patch.auth_type && patch.auth_type !== "refresh_token") forbidden.push("auth_type"); + if (patch.type && patch.type !== current.type) forbidden.push("type"); + if (forbidden.length > 0) { + throw new Error( + `Cannot mutate ${forbidden.join(", ")} on a refresh_token config (id=${id}). ` + + `Use set_refresh_token_mcp or start_dcr_oauth_flow to re-provision.`, + ); + } + + if (patch.enabled !== undefined && patch.enabled !== current.enabled) { + this.ctx.storage.sql.exec( + "UPDATE mcp_configs SET enabled = ?, name = ?, updated_at = ? WHERE id = ?", + patch.enabled ? 1 : 0, + patch.name ?? current.name, + now, + id, + ); + } else if (patch.name && patch.name !== current.name) { + this.ctx.storage.sql.exec( + "UPDATE mcp_configs SET name = ?, updated_at = ? WHERE id = ?", + patch.name, + now, + id, + ); + } + + const updated = this.getMcpConfigSafe(id); + return { ...updated, headerKeys: updated.headerKeys ?? [] }; + } + let headerKeys: string[]; if (patch.headers) { // Delete old header secrets @@ -2745,29 +2812,34 @@ export class UserControl extends DurableObject { const nowSec = Math.floor(Date.now() / 1000); const expiresAt = tokens.expires_in ? nowSec + tokens.expires_in : 0; - // Store via the existing refresh-token MCP upsert path. Idempotent on - // mcpUrl, so re-running the dance just rotates the tokens in place. - const result = await this.upsertRefreshTokenMcp( - { - name: pending.mcpName, - url: pending.mcpUrl, - tokenEndpoint: pending.tokenEndpoint, - clientId: pending.clientId, - accessToken: tokens.access_token, - refreshToken: tokens.refresh_token, - expiresAt, - enabled: true, - }, - ownerEmail, - ); - - // Clean up the pending state — it's single-use. - this.ctx.storage.sql.exec( - "DELETE FROM encrypted_secrets WHERE key = ?", - `oauth_dcr_pending:${input.state}`, - ); - - return { id: result.id, name: result.name, url: result.url }; + // Pending state is single-use whether the upsert succeeds or fails — a + // captured `code` can only be redeemed once at the OAuth provider, so + // leaving the row in place after an upsert error gains the user + // nothing (the code is dead) and just leaks ciphertext until the 10-min + // TTL elapses. Use try/finally to make the cleanup unconditional. + try { + // Store via the existing refresh-token MCP upsert path. Idempotent on + // mcpUrl, so re-running the dance just rotates the tokens in place. + const result = await this.upsertRefreshTokenMcp( + { + name: pending.mcpName, + url: pending.mcpUrl, + tokenEndpoint: pending.tokenEndpoint, + clientId: pending.clientId, + accessToken: tokens.access_token, + refreshToken: tokens.refresh_token, + expiresAt, + enabled: true, + }, + ownerEmail, + ); + return { id: result.id, name: result.name, url: result.url }; + } finally { + this.ctx.storage.sql.exec( + "DELETE FROM encrypted_secrets WHERE key = ?", + `oauth_dcr_pending:${input.state}`, + ); + } } public async createUserMcpToken(email: string, label?: string): Promise<{ token: string; created_at: number }> { diff --git a/test/refresh-token-mcp-unit.test.ts b/test/refresh-token-mcp-unit.test.ts index 9acc526..117f72f 100644 --- a/test/refresh-token-mcp-unit.test.ts +++ b/test/refresh-token-mcp-unit.test.ts @@ -296,4 +296,174 @@ describe("refresh-token MCP — access token retrieval", () => { const tokenRes = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}/access-token`); expect(tokenRes.status).toBe(502); }); + + it("/mcp-configs/:id/test injects the refreshed bearer for refresh_token configs", async () => { + // Regression: before the audit fix, /test used resolveMcpConfigHeaders + // which doesn't know about refresh_token configs, so it built the + // HttpMcpClient with no Authorization header and every Test click on + // a refresh_token config returned a 401. + const url = "https://refresh-target-testable.example.com/mcp"; + await userControlFetch("/refresh-token-mcp", { + method: "POST", + body: JSON.stringify({ + name: "Testable Target", + url, + tokenEndpoint: TOKEN_ENDPOINT, + clientId: "client-testable", + accessToken: "tok-testable", + refreshToken: "rt-testable", + expiresAt: Math.floor(Date.now() / 1000) + 3600, + }), + headers: { "content-type": "application/json" }, + }); + const list = (await (await userControlFetch("/mcp-configs")).json()) as { + configs: Array<{ id: string; url?: string }>; + }; + const id = list.configs.find((c) => c.url === url)!.id; + + // Mock fetch to capture the Authorization header used on the MCP + // connection attempt. We return an unparseable response so the MCP + // client fails fast — we don't care about the connection success, + // only about the request shape. + let authHeader: string | null = null; + globalThis.fetch = (async (_input: RequestInfo | URL, init?: RequestInit) => { + const headers = new Headers(init?.headers ?? {}); + if (headers.has("authorization")) authHeader = headers.get("authorization"); + return new Response("not-mcp", { status: 200 }); + }) as unknown as typeof fetch; + + const res = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}/test`, { + method: "POST", + }); + expect(res.status).toBe(200); + // Whatever the connection result, the request must have carried the + // bearer pulled from encrypted_secrets. + expect(authHeader).toBe("Bearer tok-testable"); + }); + + it("PUT /mcp-configs/:id refuses to mutate headers/url/auth_type on a refresh_token config", async () => { + // Regression: before the audit fix, updateMcpConfigEncrypted treated + // refresh_token rows the same as static_headers, so a PUT with + // `headers` would wipe encrypted_secrets (destroying the token chain) + // and a PUT with `auth_type: "static_headers"` would silently downgrade. + const url = "https://refresh-target-protected.example.com/mcp"; + await userControlFetch("/refresh-token-mcp", { + method: "POST", + body: JSON.stringify({ + name: "Protected Target", + url, + tokenEndpoint: TOKEN_ENDPOINT, + clientId: "client-protected", + accessToken: "tok-protected", + refreshToken: "rt-protected", + expiresAt: Math.floor(Date.now() / 1000) + 3600, + }), + headers: { "content-type": "application/json" }, + }); + const list = (await (await userControlFetch("/mcp-configs")).json()) as { + configs: Array<{ id: string; url?: string }>; + }; + const id = list.configs.find((c) => c.url === url)!.id; + + // PUT headers — must be rejected. + const res1 = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}`, { + method: "PUT", + body: JSON.stringify({ headers: { Authorization: "Bearer evil" } }), + headers: { "content-type": "application/json" }, + }); + expect(res1.status).toBeGreaterThanOrEqual(400); + + // PUT auth_type downgrade — must be rejected. + const res2 = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}`, { + method: "PUT", + body: JSON.stringify({ auth_type: "static_headers" }), + headers: { "content-type": "application/json" }, + }); + expect(res2.status).toBeGreaterThanOrEqual(400); + + // PUT enabled — must be ALLOWED. This is the one mutation a UI + // toggle has to work for. + const res3 = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}`, { + method: "PUT", + body: JSON.stringify({ enabled: false }), + headers: { "content-type": "application/json" }, + }); + expect(res3.status).toBe(200); + + // Token chain still intact — a follow-up access-token read works + // without needing a refresh (no fetch should be called since the + // cached token is still valid). + let fetchCalled = false; + globalThis.fetch = (async () => { + fetchCalled = true; + return new Response("{}", { status: 200, headers: { "content-type": "application/json" } }); + }) as unknown as typeof fetch; + const tokenRes = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}/access-token`); + expect(tokenRes.status).toBe(200); + expect(((await tokenRes.json()) as { accessToken: string }).accessToken).toBe("tok-protected"); + expect(fetchCalled).toBe(false); + }); + + it("rotates the stored refresh token — second refresh uses the rotated value", async () => { + // Regression test for the audit's blind spot: the original suite mocked + // a rotated refresh token in the first response but never asserted that + // the rotated value was what got sent on the next refresh. If + // refreshMcpAccessToken accidentally re-used the *original* refresh + // token, a real provider would return invalid_grant on the second + // refresh (because rotation invalidates the previous token). + const url = "https://refresh-target-rotation.example.com/mcp"; + await userControlFetch("/refresh-token-mcp", { + method: "POST", + body: JSON.stringify({ + name: "Rotation Target", + url, + tokenEndpoint: TOKEN_ENDPOINT, + clientId: "client-rotation", + accessToken: "access-v0", + refreshToken: "refresh-v0", + expiresAt: Math.floor(Date.now() / 1000) - 1, + }), + headers: { "content-type": "application/json" }, + }); + const list = (await (await userControlFetch("/mcp-configs")).json()) as { + configs: Array<{ id: string; url?: string }>; + }; + const id = list.configs.find((c) => c.url === url)!.id; + + let call = 0; + const sentRefreshTokens: string[] = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const bodyStr = typeof init?.body === "string" + ? init.body + : init?.body instanceof URLSearchParams + ? init.body.toString() + : ""; + const match = bodyStr.match(/refresh_token=([^&]+)/); + if (match) sentRefreshTokens.push(decodeURIComponent(match[1])); + call += 1; + const responses = [ + { access_token: "access-v1", refresh_token: "refresh-v1", expires_in: -1 }, + { access_token: "access-v2", refresh_token: "refresh-v2", expires_in: 900 }, + ]; + const payload = responses[call - 1] ?? responses[responses.length - 1]; + return new Response(JSON.stringify(payload), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + // First refresh — should send refresh-v0 and rotate to refresh-v1. + const first = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}/access-token`); + expect(first.status).toBe(200); + expect((await first.json() as { accessToken: string }).accessToken).toBe("access-v1"); + + // Second refresh — should send refresh-v1 (the rotated value), not v0. + // expires_in=-1 on the first response means the cached token is already + // expired, so the access-token endpoint forces another refresh. + const second = await userControlFetch(`/mcp-configs/${encodeURIComponent(id)}/access-token`); + expect(second.status).toBe(200); + expect((await second.json() as { accessToken: string }).accessToken).toBe("access-v2"); + + expect(sentRefreshTokens).toEqual(["refresh-v0", "refresh-v1"]); + }); });