diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 2ae7cece6a2..81fb627187e 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 be19be0af04..b67c1f9087c 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) 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 832811b281a..878bde476f2 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 185086fe60a..fa0db99864c 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 6fb15c45947..e7ac56430c4 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 @@ -210,11 +225,39 @@ 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() 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 +265,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 16d6a2d4672..841a82a9998 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(() => {