From 1a3ba6566dddfdcdac8ea13bb025f039bfd99baa Mon Sep 17 00:00:00 2001 From: Devin Collins <3997333+ImDevinC@users.noreply.github.com> Date: Fri, 15 May 2026 13:31:33 -0700 Subject: [PATCH 1/2] fix(mcp): handle oauth flows that connect without tokens Treat MCP servers that can connect their transport without actually persisting OAuth credentials as still needing authentication. This keeps MCP auth state, CLI messaging, and the explicit startAuth flow aligned for issue #5953. Fixes #5953 Tested: - bun test test/mcp/oauth-auto-connect.test.ts test/mcp/oauth-browser.test.ts test/mcp/lifecycle.test.ts - bun typecheck - reviewed the MCP-only diff against CONTRIBUTING.md and .github/pull_request_template.md to confirm the change stays scoped to the auth fix and includes targeted regression coverage Co-authored-by: opencode --- packages/opencode/src/cli/cmd/mcp.ts | 3 + packages/opencode/src/mcp/auth.ts | 21 ++++++- packages/opencode/src/mcp/index.ts | 59 ++++++++++++++----- packages/opencode/test/mcp/lifecycle.test.ts | 1 + .../test/mcp/oauth-auto-connect.test.ts | 46 ++++++++++++++- .../opencode/test/mcp/oauth-browser.test.ts | 12 ++++ 6 files changed, 121 insertions(+), 21 deletions(-) diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 2ae7cece6a27..81fb627187ee 100644 --- a/packages/opencode/src/cli/cmd/mcp.ts +++ b/packages/opencode/src/cli/cmd/mcp.ts @@ -271,6 +271,9 @@ export const McpAuthCommand = effectCmd({ Effect.sync(() => { if (status.status === "connected") { spinner.stop("Authentication successful!") + } else if (status.status === "needs_auth") { + spinner.stop("Authentication did not complete", 1) + prompts.log.error("Server connected without storing OAuth credentials") } else if (status.status === "needs_client_registration") { spinner.stop("Authentication failed", 1) prompts.log.error(status.error) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index be19be0af04e..371217ccb10a 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -33,6 +33,11 @@ type AuthData = Record const filepath = path.join(Global.Path.data, "mcp-auth.json") +function normalizeEntry(entry: Entry | undefined) { + if (!entry) return + if (entry.tokens || entry.clientInfo || entry.codeVerifier || entry.oauthState || entry.serverUrl) return entry +} + export interface Interface { readonly all: () => Effect.Effect> readonly get: (mcpName: string) => Effect.Effect @@ -58,7 +63,12 @@ export const layer = Layer.effect( const all = Effect.fn("McpAuth.all")(function* () { return yield* fs.readJson(filepath).pipe( - Effect.map((data): AuthData => Option.getOrElse(decodeAuthData(data), () => ({}) as AuthData) as AuthData), + Effect.map((data): AuthData => { + const decoded = Option.getOrElse(decodeAuthData(data), () => ({}) as AuthData) as AuthData + return Object.fromEntries( + Object.entries(decoded).flatMap(([mcpName, entry]) => (normalizeEntry(entry) ? [[mcpName, entry]] : [])), + ) + }), Effect.catch(() => Effect.succeed({} as AuthData)), ) }) @@ -78,8 +88,13 @@ export const layer = Layer.effect( const set = Effect.fn("McpAuth.set")(function* (mcpName: string, entry: Entry, serverUrl?: string) { const data = yield* all() - if (serverUrl) entry.serverUrl = serverUrl - yield* fs.writeJson(filepath, { ...data, [mcpName]: entry }, 0o600).pipe(Effect.orDie) + const next = normalizeEntry(serverUrl ? { ...entry, serverUrl } : entry) + if (!next) { + delete data[mcpName] + yield* fs.writeJson(filepath, data, 0o600).pipe(Effect.orDie) + return + } + yield* fs.writeJson(filepath, { ...data, [mcpName]: next }, 0o600).pipe(Effect.orDie) }) const remove = Effect.fn("McpAuth.remove")(function* (mcpName: string) { diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 832811b281a5..878bde476f28 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -3,7 +3,8 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js" import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js" import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js" import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js" -import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js" +import { auth as performOAuth, UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js" +import { createFetchWithInit } from "@modelcontextprotocol/sdk/shared/transport.js" import { CallToolResultSchema, ListToolsResultSchema, @@ -756,7 +757,7 @@ export const layer = Layer.effect( return mcpConfig }) - const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { + const beginAuth = Effect.fn("MCP.beginAuth")(function* (mcpName: string) { const mcpConfig = yield* getMcpConfig(mcpName) if (!mcpConfig) throw new Error(`MCP server ${mcpName} not found or disabled`) if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) @@ -792,7 +793,24 @@ export const layer = Layer.effect( auth, ) - const transport = new StreamableHTTPClientTransport(url, { authProvider }) + const requestInit = mcpConfig.headers ? { headers: mcpConfig.headers } : undefined + const transport = new StreamableHTTPClientTransport(url, { authProvider, requestInit }) + + const authorization = yield* Effect.tryPromise({ + try: () => + performOAuth(authProvider, { + serverUrl: url, + scope: oauthConfig?.scope, + fetchFn: createFetchWithInit(undefined, requestInit), + }), + catch: (error) => (error instanceof Error ? error : new Error(String(error))), + }) + + if (authorization === "REDIRECT") { + if (!capturedUrl) throw new Error(`OAuth redirect requested without authorization URL for MCP server: ${mcpName}`) + pendingOAuthTransports.set(mcpName, transport) + return { authorizationUrl: capturedUrl.toString(), oauthState } satisfies AuthResult + } return yield* Effect.tryPromise({ try: () => { @@ -802,19 +820,11 @@ export const layer = Layer.effect( .then(() => ({ authorizationUrl: "", oauthState, client }) satisfies AuthResult) }, catch: (error) => error, - }).pipe( - Effect.catch((error) => { - if (error instanceof UnauthorizedError && capturedUrl) { - pendingOAuthTransports.set(mcpName, transport) - return Effect.succeed({ authorizationUrl: capturedUrl.toString(), oauthState } satisfies AuthResult) - } - return Effect.die(error) - }), - ) + }) }) - const authenticate = Effect.fn("MCP.authenticate")(function* (mcpName: string) { - const result = yield* startAuth(mcpName) + const authenticateImpl = Effect.fn("MCP.authenticate")(function* (mcpName: string) { + const result = yield* beginAuth(mcpName) if (!result.authorizationUrl) { const client = "client" in result ? result.client : undefined const mcpConfig = yield* getMcpConfig(mcpName) @@ -829,8 +839,19 @@ export const layer = Layer.effect( return { status: "failed", error: "Failed to get tools" } as Status } - const s = yield* InstanceState.get(state) yield* auth.clearOAuthState(mcpName) + yield* auth.clearCodeVerifier(mcpName) + const authStatus = yield* getAuthStatus(mcpName) + if (authStatus !== "authenticated") { + log.warn("oauth authenticate completed without stored tokens", { mcpName, authStatus }) + yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + return { + status: "failed", + error: "Server connected without requesting OAuth. This MCP endpoint may not support interactive OAuth authentication.", + } as Status + } + + const s = yield* InstanceState.get(state) return yield* storeClient(s, mcpName, client, listed, mcpConfig.timeout) } @@ -871,6 +892,14 @@ export const layer = Layer.effect( return yield* finishAuth(mcpName, code) }) + const startAuth = (mcpName: string) => + beginAuth(mcpName).pipe( + Effect.map((result) => ({ authorizationUrl: result.authorizationUrl, oauthState: result.oauthState })), + Effect.orDie, + ) + + const authenticate = (mcpName: string) => authenticateImpl(mcpName).pipe(Effect.orDie) + const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { const transport = pendingOAuthTransports.get(mcpName) if (!transport) throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) diff --git a/packages/opencode/test/mcp/lifecycle.test.ts b/packages/opencode/test/mcp/lifecycle.test.ts index 185086fe60a9..fa0db99864c6 100644 --- a/packages/opencode/test/mcp/lifecycle.test.ts +++ b/packages/opencode/test/mcp/lifecycle.test.ts @@ -110,6 +110,7 @@ void mock.module("@modelcontextprotocol/sdk/client/auth.js", () => ({ super("Unauthorized") } }, + auth: async () => "AUTHORIZED", })) // Mock Client that delegates to per-name MockClientState diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index 6fb15c45947a..0a55d7a88222 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -21,6 +21,7 @@ const transportCalls: Array<{ // auth flow (which calls provider.state()) or a simple UnauthorizedError. let simulateAuthFlow = true let connectSucceedsImmediately = false +let directAuthResult: "REDIRECT" | "AUTHORIZED" = "REDIRECT" // Mock the transport constructors to simulate OAuth auto-auth on 401 void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ @@ -102,12 +103,26 @@ void mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ // Mock UnauthorizedError in the auth module so instanceof checks work void mock.module("@modelcontextprotocol/sdk/client/auth.js", () => ({ UnauthorizedError: MockUnauthorizedError, + auth: async ( + provider: { + state?: () => Promise + saveCodeVerifier?: (v: string) => Promise + redirectToAuthorization?: (url: URL) => Promise + }, + ) => { + if (directAuthResult === "AUTHORIZED") return "AUTHORIZED" + await provider.state?.() + await provider.saveCodeVerifier?.("test-verifier") + await provider.redirectToAuthorization?.(new URL("https://auth.example.com/authorize?state=test")) + return "REDIRECT" + }, })) beforeEach(() => { transportCalls.length = 0 simulateAuthFlow = true connectSucceedsImmediately = false + directAuthResult = "REDIRECT" }) // Import modules after mocking @@ -211,10 +226,11 @@ mcpTest.instance("state() returns existing state when one is saved", () => ) mcpTest.instance( - "authenticate() stores a connected client when auth completes without redirect", + "authenticate() fails clearly when transport reconnects without stored tokens", () => MCP.Service.use((mcp) => Effect.gen(function* () { + const auth = yield* McpAuth.Service const added = yield* mcp.add("test-oauth-connect", { type: "remote", url: "https://example.com/mcp", @@ -222,15 +238,39 @@ mcpTest.instance( const before = added.status as Record expect(before["test-oauth-connect"]?.status).toBe("needs_auth") + directAuthResult = "AUTHORIZED" simulateAuthFlow = false connectSucceedsImmediately = true const result = yield* mcp.authenticate("test-oauth-connect") - expect(result.status).toBe("connected") + expect(result.status).toBe("failed") + if (result.status === "failed") { + expect(result.error).toContain("may not support interactive OAuth authentication") + } + + const entry = yield* auth.get("test-oauth-connect") + expect(entry).toBeUndefined() const after = yield* mcp.status() - expect(after["test-oauth-connect"]?.status).toBe("connected") + expect(after["test-oauth-connect"]?.status).toBe("needs_auth") }), ), { config: config("test-oauth-connect") }, ) + +mcpTest.instance( + "startAuth() returns an authorization URL even when transport can connect anonymously", + () => + MCP.Service.use((mcp) => + Effect.gen(function* () { + simulateAuthFlow = false + connectSucceedsImmediately = true + directAuthResult = "REDIRECT" + + const result = yield* mcp.startAuth("test-oauth-direct") + expect(result.authorizationUrl).toContain("https://auth.example.com/authorize") + expect(result.oauthState.length).toBeGreaterThan(0) + }), + ), + { config: config("test-oauth-direct") }, +) diff --git a/packages/opencode/test/mcp/oauth-browser.test.ts b/packages/opencode/test/mcp/oauth-browser.test.ts index 16d6a2d46720..841a82a99985 100644 --- a/packages/opencode/test/mcp/oauth-browser.test.ts +++ b/packages/opencode/test/mcp/oauth-browser.test.ts @@ -95,6 +95,18 @@ void mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ // Mock UnauthorizedError in the auth module void mock.module("@modelcontextprotocol/sdk/client/auth.js", () => ({ UnauthorizedError: MockUnauthorizedError, + auth: async ( + provider: { + state?: () => Promise + saveCodeVerifier?: (v: string) => Promise + redirectToAuthorization?: (url: URL) => Promise + }, + ) => { + await provider.state?.() + await provider.saveCodeVerifier?.("test-verifier") + await provider.redirectToAuthorization?.(new URL("https://auth.example.com/authorize?client_id=test")) + return "REDIRECT" + }, })) beforeEach(() => { From 2879bc5b1983aed790164aa569bb7d7afeaa0a91 Mon Sep 17 00:00:00 2001 From: Devin Collins <3997333+ImDevinC@users.noreply.github.com> Date: Sat, 16 May 2026 02:02:53 -0700 Subject: [PATCH 2/2] fix(mcp): drop stale oauth entries without credentials Stop persisting MCP auth records when only the server URL remains after credentials are invalidated. This keeps logout state aligned with actual OAuth material during the issue #5953 fix. Tested: - bun test test/mcp/oauth-auto-connect.test.ts - bun typecheck Co-authored-by: opencode --- packages/opencode/src/mcp/auth.ts | 2 +- .../test/mcp/oauth-auto-connect.test.ts | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index 371217ccb10a..b67c1f9087c6 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -35,7 +35,7 @@ const filepath = path.join(Global.Path.data, "mcp-auth.json") function normalizeEntry(entry: Entry | undefined) { if (!entry) return - if (entry.tokens || entry.clientInfo || entry.codeVerifier || entry.oauthState || entry.serverUrl) return entry + if (entry.tokens || entry.clientInfo || entry.codeVerifier || entry.oauthState) return entry } export interface Interface { diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index 0a55d7a88222..e7ac56430c47 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -225,6 +225,33 @@ mcpTest.instance("state() returns existing state when one is saved", () => }), ) +mcpTest.instance("invalidateCredentials() removes entries that only retain serverUrl", () => + Effect.gen(function* () { + const auth = yield* McpAuth.Service + const provider = new McpOAuthProvider( + "test-stale-server-url", + "https://example.com/mcp", + {}, + { onRedirect: async () => {} }, + auth, + ) + + yield* Effect.promise(() => + provider.saveTokens({ + access_token: "token", + token_type: "Bearer", + }), + ) + + expect((yield* auth.get("test-stale-server-url"))?.serverUrl).toBe("https://example.com/mcp") + + yield* Effect.promise(() => provider.invalidateCredentials("tokens")) + + expect(yield* auth.get("test-stale-server-url")).toBeUndefined() + expect((yield* auth.all())["test-stale-server-url"]).toBeUndefined() + }), +) + mcpTest.instance( "authenticate() fails clearly when transport reconnects without stored tokens", () =>