From db9af4b26bccf91b389792a06b6ceb4022ea6cd7 Mon Sep 17 00:00:00 2001 From: jonnyparris <6400000+jonnyparris@users.noreply.github.com> Date: Tue, 26 May 2026 17:05:04 +0100 Subject: [PATCH] fix(mcp): remove github catalog entry + render orphaned OAuth servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanup fixes for the OAuth integrations UI: 1. Remove GitHub from CORE_MCP_CATALOG (api.githubcopilot.com/mcp/) GitHub Copilot's MCP server returns 'Incompatible auth server: does not support dynamic client registration' when the SDK-managed OAuth path attempts DCR. The catalog entry was advertising a Connect with OAuth button that could never succeed. Users who want GitHub MCP should add a github_token secret instead β€” the existing static- headers path already handles it and the UI hides any GitHub catalog suggestion when a github_token is present. The fallback github_token-hides-suggestion code path in dodo-settings.js (`if(cat.id==="github"&&hasGithubToken)return`) becomes dead with this change but is harmless to leave in place. 2. Always render orphaned OAuth servers Before this commit, renderOAuthCard was only called for catalog entries with auth_type='oauth' that had a matching SDK-managed hub-DO record. When a catalog entry was removed (e.g. cf-portal in PR #89) but the user already had a failed/stuck server record from a previous attempt, the record was invisible in the UI β†’ no way to clear it without poking /api/mcp/delete-auth manually. Fix: track which oauthServers got rendered alongside a catalog entry; render any unmatched ones via the new renderOrphanedOAuthCard with a 'Clear' button (calls existing /api/mcp/delete-auth). Also improve renderOAuthCard for non-ready states: - 'Refresh' button only shows for state=ready (it was retrying against a broken config in other states, which was useless) - The remove button reads 'Disconnect' when ready, 'Clear' otherwise. Tests: 832/832 pass. Catalog test count updated to reflect the removed github entry; new assertions confirm github and cf-portal are NOT in the catalog so the next person doesn't accidentally re-add them without reading the history. Stale UI rendering of an orphaned cf-portal 'authenticating' entry that surfaced this issue was already wiped manually via /api/mcp/delete-auth. beep-boop-πŸ€– --- public/js/dodo-settings.js | 48 +++++++++++++++++++++++++++++++---- src/mcp-catalog.ts | 20 +++++++-------- test/mcp-catalog-unit.test.ts | 11 +++++--- test/mcp-config.test.ts | 17 +++++++++---- 4 files changed, 71 insertions(+), 25 deletions(-) diff --git a/public/js/dodo-settings.js b/public/js/dodo-settings.js index 9f899c4..15b956f 100644 --- a/public/js/dodo-settings.js +++ b/public/js/dodo-settings.js @@ -255,6 +255,12 @@ function renderIntegrations(){ // Match catalog entries to OAuth-connected servers by hostname. const oauthByHostname=new Map(); oauthServers.forEach(s=>{const h=getHostname(s.url);if(h)oauthByHostname.set(h,s);}); + // Track which oauthServers we've rendered as part of a catalog entry so + // the leftover loop below can pick up the orphans. Without this, a + // hub-DO record for an MCP URL that's no longer in the catalog (e.g. a + // failed cf-portal attempt left behind after the catalog entry was + // removed) would never render β†’ user has no UI handle to clear it. + const renderedOAuthServerIds=new Set(); const connected=[],suggestions=[]; const hasGithubToken=secretKeys.includes("github_token"); mcpCatalog.forEach(cat=>{ @@ -271,6 +277,7 @@ function renderIntegrations(){ for(const h of catHosts){if(oauthByHostname.has(h)){oauthServer=oauthByHostname.get(h);break;}} if(oauthServer){ connected.push(renderOAuthCard(cat,oauthServer)); + renderedOAuthServerIds.add(oauthServer.id); }else{ suggestions.push(renderOAuthCard(cat,null)); } @@ -298,6 +305,16 @@ function renderIntegrations(){ connected.push(renderIntegCard(cfg.name,cfg.url||"Custom integration",null,cfg)); } }); + // Render any orphaned OAuth servers β€” entries in the per-user hub DO + // whose URL doesn't match any current catalog entry. Usually leftovers + // from a failed OAuth attempt where the catalog entry has since been + // removed, or one-off MCP servers added directly via /api/mcp/start-auth. + // Surfacing them gives the user a "Clear" button instead of letting the + // zombie linger. + oauthServers.forEach(s=>{ + if(renderedOAuthServerIds.has(s.id))return; + connected.push(renderOrphanedOAuthCard(s)); + }); const cards=[...connected,...suggestions]; $("integrations-list").innerHTML=cards.length?cards.join(""):'
No integrations
'; } @@ -346,10 +363,12 @@ async function refreshIntegrationToken(id,displayName){ }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 -// in the user's hub DO, not in mcp_configs. +// OAuth-MCP catalog entries render with different actions based on the +// server's current state in the per-user hub DO. The Agents SDK transitions +// between authenticating β†’ connecting β†’ discovering β†’ ready (success path) +// or β†’ failed (terminal error). For terminal states the only sensible +// action is to clear the entry; offering "Refresh" on a failed server +// retries against the same broken config and confuses users. function renderOAuthCard(cat,server){ const desc=cat.description||""; let statusHtml,actionsHtml; @@ -362,7 +381,10 @@ function renderOAuthCard(cat,server){ :server.state; const color=server.state==="ready"?"var(--text-success)":server.state==="failed"?"var(--text-error,#b91c1c)":"var(--text-subtle)"; statusHtml=`${esc(stateLabel)}`; - actionsHtml=``; + const isReady=server.state==="ready"; + const refreshBtn=isReady?``:""; + const removeLabel=isReady?"Disconnect":"Clear"; + actionsHtml=`${refreshBtn}`; }else{ statusHtml=`Not connected`; actionsHtml=``; @@ -370,6 +392,22 @@ function renderOAuthCard(cat,server){ return `
${esc(cat.name)}
${esc(desc)}
${statusHtml}
${actionsHtml}
`; } +// Render an OAuth server that has no matching catalog entry β€” either a +// custom MCP added via `/api/mcp/start-auth`, or a leftover from a failed +// attempt whose catalog entry has since been removed. Always offer a +// "Clear" action so the user can wipe it; never offer "Refresh" since we +// don't know what the original setup looked like. +function renderOrphanedOAuthCard(server){ + const stateLabel=server.state==="ready"?`Connected β€” ${server.toolCount} tool${server.toolCount!==1?'s':''}` + :server.state==="authenticating"?"Authenticating… (no longer reachable)" + :server.state==="failed"?(server.error?`Failed: ${server.error}`:"Failed") + :server.state; + const color=server.state==="ready"?"var(--text-success)":server.state==="failed"?"var(--text-error,#b91c1c)":"var(--text-subtle)"; + const displayName=server.name||"OAuth MCP"; + const desc=`${server.url} β€” orphaned entry, not in catalog`; + return `
${esc(displayName)}
${esc(desc)}
${esc(stateLabel)}
`; +} + // Kick off the OAuth dance for a catalog entry. The server returns either // {authUrl} (popup needed) or {message:"Connected"} (already authenticated). // The popup polls /api/mcp/oauth-servers after closing to detect the new diff --git a/src/mcp-catalog.ts b/src/mcp-catalog.ts index 255d872..74c277b 100644 --- a/src/mcp-catalog.ts +++ b/src/mcp-catalog.ts @@ -25,17 +25,15 @@ const CORE_MCP_CATALOG: McpCatalogEntry[] = [ auth_type: "static_headers", knownHosts: [], }, - { - id: "github", - name: "GitHub", - description: - "Structured tools for issues, PRs, actions, and code scanning (beyond basic git operations)", - url: "https://api.githubcopilot.com/mcp/", - setupGuide: - "Use the remote server at https://api.githubcopilot.com/mcp/ with OAuth, or deploy locally with a PAT", - auth_type: "oauth", - knownHosts: ["api.githubcopilot.com"], - }, + // NOTE: GitHub Copilot's MCP server at https://api.githubcopilot.com/mcp/ + // was tested and is intentionally NOT in this catalog as an OAuth entry. + // Its OAuth provider does NOT support Dynamic Client Registration β€” + // attempting to connect via the SDK-managed OAuth path errors with + // "Incompatible auth server: does not support dynamic client registration". + // To use GitHub MCP, add a github_token secret (Personal Access Token or + // GitHub App installation token); the existing static-headers path picks + // it up automatically and the UI hides the GitHub catalog entry when one + // is present. ]; const DEFAULT_CLOUDFLARE_REMOTE_MCP_CATALOG: McpCatalogEntry[] = [ diff --git a/test/mcp-catalog-unit.test.ts b/test/mcp-catalog-unit.test.ts index 45768d0..54ae54c 100644 --- a/test/mcp-catalog-unit.test.ts +++ b/test/mcp-catalog-unit.test.ts @@ -23,11 +23,14 @@ describe("MCP_CATALOG", () => { expect(entry?.knownHosts).toContain("browser.mcp.cloudflare.com"); }); - it("includes github as an OAuth catalog entry", () => { + it("does NOT include github as an OAuth catalog entry β€” GitHub's MCP server doesn't support DCR", () => { + // Documented in mcp-catalog.ts. api.githubcopilot.com returns + // "Incompatible auth server: does not support dynamic client + // registration" when the SDK-managed OAuth path attempts DCR. To use + // GitHub MCP, add a github_token secret β€” the static-headers path + // already handles it and the UI hides the suggestion when present. const entry = MCP_CATALOG.find((e) => e.id === "github"); - expect(entry).toBeDefined(); - expect(entry?.auth_type).toBe("oauth"); - expect(entry?.knownHosts).toContain("api.githubcopilot.com"); + expect(entry).toBeUndefined(); }); it("every catalog entry has either auth_type or is the self-MCP", () => { diff --git a/test/mcp-config.test.ts b/test/mcp-config.test.ts index 6c47c98..9a55f5e 100644 --- a/test/mcp-config.test.ts +++ b/test/mcp-config.test.ts @@ -182,22 +182,29 @@ describe("Approved MCP catalog admin operations", () => { }); describe("MCP Catalog", () => { - it("returns catalog with at least 3 entries", async () => { + it("returns catalog with the expected default entries", async () => { const res = await fetchJson("/api/mcp-catalog"); expect(res.status).toBe(200); const catalog = (await res.json()) as Array<{ id: string; name: string; description: string; url: string; setupGuide: string }>; expect(Array.isArray(catalog)).toBe(true); - // Catalog seed is config-driven (PR #48). Three entries land by default β€” - // dodo-self, github, browser-rendering β€” and admins can add more via + // Catalog seed is config-driven. The current defaults are + // dodo-self + browser-rendering. cf-portal is omitted (loopback-only + // OAuth β€” see mcp-catalog.ts comment) and github is omitted (its MCP + // server doesn't support DCR). Admins can add more via // /api/admin/approved-mcps. - expect(catalog.length).toBeGreaterThanOrEqual(3); + expect(catalog.length).toBeGreaterThanOrEqual(2); // Verify known entries exist const ids = catalog.map((c) => c.id); expect(ids).toContain("dodo-self"); - expect(ids).toContain("github"); expect(ids).toContain("browser-rendering"); + // And confirm the catalog entries removed for compatibility reasons + // are indeed gone, so the next person to add them doesn't have to + // re-read all the audit history. + expect(ids).not.toContain("github"); + expect(ids).not.toContain("cf-portal"); + // Verify each entry has required fields for (const entry of catalog) { expect(entry.id).toBeTruthy();