diff --git a/apps/app/src/app/constants.ts b/apps/app/src/app/constants.ts index 84c0c715..41ca25a7 100644 --- a/apps/app/src/app/constants.ts +++ b/apps/app/src/app/constants.ts @@ -1,5 +1,6 @@ import type { ModelRef, SuggestedPlugin } from "./types"; import { t } from "../i18n"; +import { deriveMcpServerName } from "./mcp-identity"; import { BUILT_IN_LEGALWORK_EXTENSION_MANIFESTS, extensionContribution, @@ -133,13 +134,9 @@ export function isBuiltInLegalWorkExtension(entry: Pick): string { if (entry.serverName) return entry.serverName; - return entry.name - .toLowerCase() - .replace(/[^a-z0-9_-]/g, "-") - .replace(/-+/g, "-") - .replace(/^-|-$/g, "") || "mcp"; + return deriveMcpServerName(entry.name); } export const MCP_QUICK_CONNECT: McpDirectoryInfo[] = [ diff --git a/apps/app/src/app/mcp-auth-state.ts b/apps/app/src/app/mcp-auth-state.ts new file mode 100644 index 00000000..5eff40e9 --- /dev/null +++ b/apps/app/src/app/mcp-auth-state.ts @@ -0,0 +1,89 @@ +/** + * Deciding whether logging an MCP server out actually did anything. + * + * Some remote MCP servers accept an unauthenticated handshake and reject only + * individual tool calls. The engine reports those as plain `connected`, exactly + * like an authenticated one — `McpStatusConnected` carries no auth state, and no + * endpoint reports stored credentials. So a connector that was never signed in + * shows as Ready, "Log out" clears a credential that does not exist, the badge + * drops to Paused, and the next launch silently restores Ready. Read as a stale + * session surviving a logout, and reported that way in issue #86. + * + * The signal is to reconnect once the credentials are gone — the same thing the + * next launch does, only now rather than later. A server that needed them comes + * back asking to sign in; one that never did comes back connected. Doing it + * during logout also leaves the badge showing the truth immediately, instead of + * a Paused state that quietly flips to Ready on restart. + */ + +export type McpStatusSnapshot = Record; + +/** Delays between status reads after the reconnect, in ms. */ +export const RECONNECT_PROBE_DELAYS_MS = [250, 500, 1000, 1500]; + +export type DetectReconnectOptions = { + /** Server name as the engine knows it. */ + serverName: string; + /** Ask the engine to reconnect the server (POST /mcp/{name}/connect). */ + reconnect: () => Promise; + /** Read current MCP statuses; return null when unavailable. */ + readStatus: () => Promise; + /** Called with each snapshot read, so callers can keep their UI current. */ + onStatus?: (statuses: McpStatusSnapshot) => void; + /** Injected for tests; defaults to a real timer. */ + wait?: (ms: number) => Promise; + /** Abort early, e.g. when the store has been disposed. */ + isCancelled?: () => boolean; + delaysMs?: number[]; +}; + +/** + * True when the server reconnects successfully after its credentials were + * removed — meaning it never authenticated and the logout cleared nothing. + * + * False for every other outcome, including a failed probe or an unreadable + * status. This picks which message a user is shown, so an uncertain answer must + * fall back to the ordinary one rather than accuse a working logout of being a + * no-op. + */ +export async function detectReconnectWithoutAuth(options: DetectReconnectOptions): Promise { + const { + serverName, + reconnect, + readStatus, + onStatus, + wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)), + isCancelled, + delaysMs = RECONNECT_PROBE_DELAYS_MS, + } = options; + + try { + await reconnect(); + } catch { + // The probe is a diagnostic, never a reason to fail the logout itself. + return false; + } + if (isCancelled?.()) return false; + + for (const delay of delaysMs) { + await wait(delay); + if (isCancelled?.()) return false; + + let statuses: McpStatusSnapshot | null; + try { + statuses = await readStatus(); + } catch { + return false; + } + if (!statuses) return false; + onStatus?.(statuses); + + const current = statuses[serverName]?.status; + if (current === "connected") return true; + // The credentials mattered: the server is asking for them again. Nothing + // later in the poll can overturn that, so stop. + if (current === "needs_auth" || current === "needs_client_registration") return false; + } + + return false; +} diff --git a/apps/app/src/app/mcp-identity.ts b/apps/app/src/app/mcp-identity.ts new file mode 100644 index 00000000..8ab0dc18 --- /dev/null +++ b/apps/app/src/app/mcp-identity.ts @@ -0,0 +1,79 @@ +/** + * The one place an MCP server's identity is derived. + * + * A connector carries two names: the display name someone reads ("Microsoft + * SharePoint", "Iron Crow") and the server name the engine and opencode.jsonc + * know it by ("sharepoint", "iron-crow"). Connect writes the second one; every + * later operation — sign-in, logout, status lookup — has to ask for exactly + * that same string or it addresses a server that does not exist. + * + * Three separate derivations used to exist. The sign-in modal's rejected any + * display name that was not already a valid server name, so clicking "Log in" + * on SharePoint, Google Workspace, Thomson Reuters HighQ, or any connector + * someone had named with a space threw "server_name must be alphanumeric" + * before the browser was ever opened — the OAuth window simply never appeared + * (issue #86). Keeping the rule in one module is what stops that from + * reappearing: a display name is slugified here, never validated as if it were + * already a server name. + */ + +/** Identity fields every connector shape carries, however it reached us. */ +export type McpIdentity = { + /** Explicit server name, when the catalog entry pins one. */ + id?: string; + /** Safe server name for opencode.jsonc, when declared. */ + serverName?: string; + /** Display name shown in the UI. */ + name: string; +}; + +/** + * Slugify a display name into a server name opencode accepts: lowercase, + * alphanumerics plus `-` and `_`, no repeated or edge dashes. + */ +export function deriveMcpServerName(displayName: string): string { + return ( + displayName + .toLowerCase() + .replace(/[^a-z0-9_-]/g, "-") + .replace(/-+/g, "-") + .replace(/^-|-$/g, "") || "mcp" + ); +} + +/** + * The server name this connector is registered under — the key in + * opencode.jsonc and the name the engine answers to. + */ +export function getMcpIdentityKey(entry: McpIdentity): string { + return entry.id ?? entry.serverName ?? deriveMcpServerName(entry.name); +} + +/** + * Assert a string is already a valid opencode server name. Use it on a name + * that is meant to be one — a config key, a derived identity key — never on a + * display name, which needs `deriveMcpServerName` instead. + */ +export function validateMcpServerName(name: string): string { + const trimmed = name.trim(); + if (!trimmed) { + throw new Error("server_name is required"); + } + if (trimmed.startsWith("-")) { + throw new Error("server_name must not start with '-'"); + } + if (!/^[A-Za-z0-9_-]+$/.test(trimmed)) { + throw new Error("server_name must be alphanumeric with '-' or '_'"); + } + return trimmed; +} + +/** + * The name to hand the engine when signing a connector in. This is the single + * function the OAuth modal uses; keeping the derivation and the validation + * composed here is what stops a display name from being validated as if it + * were a server name (issue #86). + */ +export function resolveMcpSignInName(entry: McpIdentity): string { + return validateMcpServerName(getMcpIdentityKey(entry)); +} diff --git a/apps/app/src/app/mcp.ts b/apps/app/src/app/mcp.ts index 850863f5..504d446d 100644 --- a/apps/app/src/app/mcp.ts +++ b/apps/app/src/app/mcp.ts @@ -1,36 +1,23 @@ import { applyEdits, modify, parse, printParseErrorCode } from "jsonc-parser"; import type { McpServerConfig, McpServerEntry } from "./types"; import { readOpencodeConfig, writeOpencodeConfig } from "./lib/desktop"; +import { deriveMcpServerName } from "./mcp-identity"; type McpConfigValue = Record | null | undefined; -type McpIdentity = { - id?: string; - serverName?: string; - name: string; -}; +export { + deriveMcpServerName, + getMcpIdentityKey, + resolveMcpSignInName, + validateMcpServerName, + type McpIdentity, +} from "./mcp-identity"; -export function normalizeMcpSlug(name: string): string { - return name.toLowerCase().replace(/[^a-z0-9]+/g, "-"); -} - -export function getMcpIdentityKey(entry: McpIdentity): string { - return entry.id ?? entry.serverName ?? normalizeMcpSlug(entry.name); -} - -export function validateMcpServerName(name: string): string { - const trimmed = name.trim(); - if (!trimmed) { - throw new Error("server_name is required"); - } - if (trimmed.startsWith("-")) { - throw new Error("server_name must not start with '-'"); - } - if (!/^[A-Za-z0-9_-]+$/.test(trimmed)) { - throw new Error("server_name must be alphanumeric with '-' or '_'"); - } - return trimmed; -} +/** + * Slugify a display name into a server name. Alias of `deriveMcpServerName`; + * the derivation lives in one module so connect and sign-in cannot drift apart. + */ +export const normalizeMcpSlug = deriveMcpServerName; export async function removeMcpFromConfig( projectDir: string, diff --git a/apps/app/src/i18n/locales/en.ts b/apps/app/src/i18n/locales/en.ts index b21c22ce..e2cc880a 100644 --- a/apps/app/src/i18n/locales/en.ts +++ b/apps/app/src/i18n/locales/en.ts @@ -747,6 +747,7 @@ export default { "mcp.logout_label": "OAuth", "mcp.logout_modal_message": "This will remove stored OAuth credentials for {server}. You'll need to sign in again to use this app.", "mcp.logout_modal_title": "Log out of this app?", + "mcp.logout_no_credentials": "{server} connects without signing in, so there was nothing to log out of. It will show as Ready again, and any request it rejects has to be sorted out on the server itself.", "mcp.logout_success": "Logged out of {server}.", "mcp.logout_working": "Logging out...", "mcp.name_required": "Enter a server name.", diff --git a/apps/app/src/react-app/domains/connections/mcp-auth-modal.tsx b/apps/app/src/react-app/domains/connections/mcp-auth-modal.tsx index 1d0feed4..dcb69e3c 100644 --- a/apps/app/src/react-app/domains/connections/mcp-auth-modal.tsx +++ b/apps/app/src/react-app/domains/connections/mcp-auth-modal.tsx @@ -14,7 +14,7 @@ import { import type { McpDirectoryInfo } from "@/app/constants"; import { openDesktopUrl, opencodeMcpAuth } from "@/app/lib/desktop"; import { unwrap } from "@/app/lib/opencode"; -import { validateMcpServerName } from "@/app/mcp"; +import { getMcpIdentityKey, resolveMcpSignInName } from "@/app/mcp"; import type { Client } from "@/app/types"; import { isDesktopRuntime, normalizeDirectoryPath } from "@/app/utils"; import { t } from "@/i18n"; @@ -192,8 +192,14 @@ const humanizeEngineError = (message: string): string => { return looksLikePayload ? t("mcp.auth.connect_failed_generic") : message; }; -const resolveSlug = (name: string) => - validateMcpServerName(name).toLowerCase().replace(/[^a-z0-9]+/g, "-"); + /** + * The name the engine knows this connector by — the same key connect wrote to + * opencode.jsonc. Deriving it from the display name here instead used to + * reject anything with a space ("Microsoft SharePoint", "Iron Crow") before + * the authorization URL was ever requested, so the sign-in browser never + * opened; see app/mcp-identity.ts. + */ + const resolveSlug = resolveMcpSignInName; // Drop any stored OAuth registration/tokens for this server. Best-effort: // used to recover from a stale Dynamic Client Registration that would @@ -262,7 +268,7 @@ const resolveSlug = (name: string) => let slug = ""; try { - slug = resolveSlug(props.entry.name); + slug = resolveSlug(props.entry); } catch (err) { const message = err instanceof Error ? err.message : t("mcp.auth.failed_to_start_oauth"); setError(message); @@ -354,7 +360,7 @@ const resolveSlug = (name: string) => setNeedsReload(false); setError(t("mcp.auth.slack_client_registration_required")); } else if (message.toLowerCase().includes("does not support oauth")) { - const serverSlug = props.entry.name.toLowerCase().replace(/[^a-z0-9]+/g, "-") || "server"; + const serverSlug = slug || getMcpIdentityKey(props.entry); const canAutoReload = allowAutoReload && !props.isRemoteWorkspace && !props.reloadBlocked && Boolean(props.onReloadEngine); @@ -475,7 +481,7 @@ const resolveSlug = (name: string) => await props.onReloadEngine?.(); if (cancelled) return; - const slug = resolveSlug(props.entry!.name); + const slug = resolveSlug(props.entry!); const status = await waitForMcpAvailability(slug); if (cancelled) return; @@ -583,7 +589,7 @@ const resolveSlug = (name: string) => let slug = ""; try { - slug = resolveSlug(props.entry.name); + slug = resolveSlug(props.entry); } catch (err) { const message = err instanceof Error ? err.message : t("mcp.auth.failed_to_start_oauth"); setError(message); @@ -651,7 +657,7 @@ const resolveSlug = (name: string) => let slug = ""; try { - slug = resolveSlug(props.entry.name); + slug = resolveSlug(props.entry); } catch (err) { const message = err instanceof Error ? err.message : t("mcp.auth.failed_to_start_oauth"); setError(message); diff --git a/apps/app/src/react-app/domains/connections/store.ts b/apps/app/src/react-app/domains/connections/store.ts index 57189ae0..8012469a 100644 --- a/apps/app/src/react-app/domains/connections/store.ts +++ b/apps/app/src/react-app/domains/connections/store.ts @@ -12,6 +12,7 @@ import { extensionResource } from "../../../app/extensions"; import { captureAnalyticsEvent } from "../../../app/lib/analytics"; import { captureAppError } from "../../../app/lib/app-error"; import { createClient, unwrap } from "../../../app/lib/opencode"; +import { detectReconnectWithoutAuth, type McpStatusSnapshot } from "../../../app/mcp-auth-state"; import { finishPerf, perfNow, recordPerfLog } from "../../../app/lib/perf-log"; import { mergeRuntimeMcpServer, @@ -21,6 +22,7 @@ import { } from "../../../app/lib/desktop"; import { toSessionTransportDirectory } from "../../../app/lib/session-scope"; import { + getMcpIdentityKey, parseMcpServersFromContent, removeMcpFromConfig, validateMcpServerName, @@ -869,26 +871,55 @@ export function createConnectionsStore(options: { return; } - const matchingQuickConnect = MCP_QUICK_CONNECT.find((candidate) => { - const candidateSlug = candidate.name.toLowerCase().replace(/[^a-z0-9]+/g, "-"); - return candidateSlug === entry.name || candidate.name === entry.name; - }); + // Match on the server name the catalog entry would be registered under, not + // on its slugified title: the two differ for every entry that declares a + // serverName ("Microsoft SharePoint" -> "sharepoint"). + const matchingQuickConnect = MCP_QUICK_CONNECT.find( + (candidate) => getMcpIdentityKey(candidate) === entry.name || candidate.name === entry.name, + ); + // entry.name is the key in opencode.jsonc, so it is the authoritative + // identity — pin it as serverName so the sign-in modal asks the engine for + // this exact server even when a catalog entry supplies the display name. mutateState((current) => ({ ...current, - mcpAuthEntry: - matchingQuickConnect ?? { + mcpAuthEntry: { + ...(matchingQuickConnect ?? { name: entry.name, description: "", - type: "remote", + type: "remote" as const, url: entry.config.url, oauth: true, - }, + }), + id: entry.name, + serverName: entry.name, + }, mcpAuthNeedsReload: false, mcpAuthModalOpen: true, })); } + /** + * Did the server come back "connected" on its own after its credentials were + * dropped? See app/mcp-auth-state.ts for why that is the signal. + */ + async function didReconnectWithoutAuth( + serverName: string, + activeClient: Client | null, + projectDir: string, + ): Promise { + if (!activeClient || !projectDir) return false; + + return detectReconnectWithoutAuth({ + serverName, + reconnect: () => activeClient.mcp.connect({ directory: projectDir, name: serverName }), + readStatus: async () => + unwrap(await activeClient.mcp.status({ directory: projectDir })) as McpStatusSnapshot, + onStatus: (statuses) => setStateField("mcpStatuses", statuses as McpStatusMap), + isCancelled: () => disposed, + }); + } + async function logoutMcpAuth(name: string) { const legalworkSnapshot = getLegalworkSnapshot(); const isRemoteWorkspace = @@ -954,7 +985,25 @@ export function createConnectionsStore(options: { } await refreshMcpServers(); - setStateField("mcpStatus", t("mcp.logout_success").replace("{server}", safeName)); + + // A server that accepts the unauthenticated MCP handshake reconnects on + // its own moments after the credentials are dropped, and reports + // "connected" again — it never needed them. Claiming "Logged out" there + // is a lie the next restart exposes: the connector is back to Ready and + // reads as a session that survived the logout (issue #86). Ask what + // actually happened before saying anything. + const reconnectedWithoutAuth = await didReconnectWithoutAuth( + safeName, + activeClient, + resolvedProjectDir, + ); + setStateField( + "mcpStatus", + (reconnectedWithoutAuth + ? t("mcp.logout_no_credentials") + : t("mcp.logout_success") + ).replace("{server}", safeName), + ); } catch (error) { setStateField( "mcpStatus", diff --git a/apps/app/tests/mcp-auth-identity.test.ts b/apps/app/tests/mcp-auth-identity.test.ts new file mode 100644 index 00000000..d2617fd4 --- /dev/null +++ b/apps/app/tests/mcp-auth-identity.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, test } from "bun:test"; + +import { MCP_QUICK_CONNECT, getMcpServerName, type McpDirectoryInfo } from "../src/app/constants"; +import { deriveMcpServerName, resolveMcpSignInName, validateMcpServerName } from "../src/app/mcp"; + +/** + * Issue #86: clicking "Log in" on a connector never opened the OAuth browser. + * + * The sign-in modal derived the server name by *validating* the display name + * instead of slugifying it, so any connector whose display name was not already + * a legal server name — every one with a space — threw before the authorization + * URL was requested. These tests pin the modal's identity to the key connect + * writes into opencode.jsonc. + */ + +/** What connectMcp writes to opencode.jsonc and registers with the engine. */ +const connectKey = (entry: McpDirectoryInfo) => entry.id ?? getMcpServerName(entry); + +/** What the sign-in modal asks the engine to authenticate. */ +const signInKey = resolveMcpSignInName; + +const customConnector = (name: string): McpDirectoryInfo => ({ + name, + description: "", + type: "remote", + url: "https://example.test/mcp", + oauth: true, +}); + +describe("mcp sign-in identity", () => { + test("a display name with a space still resolves to the registered server name", () => { + const entry = customConnector("Iron Crow"); + expect(connectKey(entry)).toBe("iron-crow"); + expect(signInKey(entry)).toBe("iron-crow"); + }); + + test("sign-in never rejects a display name the add-connector form accepts", () => { + for (const name of ["Iron Crow", "IronCrow AI", "Dun & Bradstreet", "My Firm's Server"]) { + const entry = customConnector(name); + expect(() => signInKey(entry)).not.toThrow(); + expect(signInKey(entry)).toBe(connectKey(entry)); + } + }); + + test("underscores survive, because that is what connect wrote", () => { + const entry = customConnector("Iron_Crow"); + expect(connectKey(entry)).toBe("iron_crow"); + expect(signInKey(entry)).toBe("iron_crow"); + }); + + test("every built-in connector can be signed into", () => { + const broken = MCP_QUICK_CONNECT.filter((entry) => { + try { + return signInKey(entry) !== connectKey(entry); + } catch { + return true; + } + }).map((entry) => entry.name); + + expect(broken).toEqual([]); + }); + + test("a declared serverName wins over the display name", () => { + const entry: McpDirectoryInfo = { ...customConnector("Microsoft SharePoint"), serverName: "sharepoint" }; + expect(signInKey(entry)).toBe("sharepoint"); + }); + + test("an explicit id wins over both", () => { + const entry: McpDirectoryInfo = { + ...customConnector("Google Workspace"), + serverName: "workspace", + id: "google-workspace", + }; + expect(signInKey(entry)).toBe("google-workspace"); + }); + + test("deriving a server name always produces one opencode accepts", () => { + for (const name of ["Iron Crow", " ", "!!!", "Dun & Bradstreet Risk Analytics", "-leading-dash-"]) { + expect(() => validateMcpServerName(deriveMcpServerName(name))).not.toThrow(); + } + }); +}); diff --git a/apps/app/tests/mcp-logout-truthfulness.test.ts b/apps/app/tests/mcp-logout-truthfulness.test.ts new file mode 100644 index 00000000..f236d0f4 --- /dev/null +++ b/apps/app/tests/mcp-logout-truthfulness.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, test } from "bun:test"; + +import { detectReconnectWithoutAuth, type McpStatusSnapshot } from "../src/app/mcp-auth-state"; + +/** + * Issue #86: a connector shows Ready without anyone signing in, "Log out" + * reports success, and after a restart it is Ready again — which reads as a + * stale session surviving the logout. + * + * What actually happens is that the server accepts an unauthenticated + * handshake, so there were never credentials to clear. Reconnecting once they + * are gone is what tells the two cases apart: a server that needed them asks to + * sign in again, one that never did comes straight back connected. Verified + * against the pinned engine — see the PR's manual verification. + */ + +const noWait = async () => {}; + +const statuses = (status: string | undefined): McpStatusSnapshot => + status === undefined ? {} : { "iron-crow": { status } }; + +const detect = (reads: Array, overrides: Record = {}) => { + let index = 0; + return detectReconnectWithoutAuth({ + serverName: "iron-crow", + reconnect: async () => true, + readStatus: async () => reads[Math.min(index++, reads.length - 1)] ?? null, + wait: noWait, + ...overrides, + }); +}; + +describe("logout truthfulness", () => { + test("a server that reconnects without credentials never needed them", async () => { + expect(await detect([statuses("connected")])).toBe(true); + }); + + test("a reconnect a beat later is still a reconnect", async () => { + expect(await detect([statuses("disabled"), statuses("disabled"), statuses("connected")])).toBe(true); + }); + + test("a server asking to sign in again means the logout worked", async () => { + expect(await detect([statuses("needs_auth")])).toBe(false); + }); + + test("needs_client_registration also means the logout worked", async () => { + expect(await detect([statuses("needs_client_registration")])).toBe(false); + }); + + test("never reaching connected within the probe means the logout worked", async () => { + expect(await detect([statuses("disabled")])).toBe(false); + }); + + test("needs_auth is terminal — a later connect does not overturn it", async () => { + expect(await detect([statuses("needs_auth"), statuses("connected")])).toBe(false); + }); + + test("the probe must actually reconnect before judging", async () => { + let reconnected = false; + await detect([statuses("connected")], { + reconnect: async () => { + reconnected = true; + return true; + }, + }); + expect(reconnected).toBe(true); + }); + + test("a reconnect the engine refuses falls back to the ordinary message", async () => { + expect( + await detect([statuses("connected")], { + reconnect: async () => { + throw new Error("McpServerNotFoundError"); + }, + }), + ).toBe(false); + }); + + test("an unreadable status falls back to the ordinary message", async () => { + expect(await detect([null])).toBe(false); + expect( + await detectReconnectWithoutAuth({ + serverName: "iron-crow", + reconnect: async () => true, + readStatus: async () => { + throw new Error("engine unavailable"); + }, + wait: noWait, + }), + ).toBe(false); + }); + + test("a server missing from the snapshot is not a silent reconnect", async () => { + expect(await detect([statuses(undefined)])).toBe(false); + }); + + test("cancellation stops the probe without accusing the logout", async () => { + expect(await detect([statuses("connected")], { isCancelled: () => true })).toBe(false); + }); + + test("each snapshot is handed back so the caller can refresh its badges", async () => { + const seen: McpStatusSnapshot[] = []; + await detect([statuses("disabled"), statuses("connected")], { + onStatus: (s: McpStatusSnapshot) => seen.push(s), + }); + expect(seen.length).toBe(2); + expect(seen[1]?.["iron-crow"]?.status).toBe("connected"); + }); +});