diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index 164b1d1f143..53b8bf646e2 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -145,7 +145,15 @@ export class McpOAuthProvider implements OAuthClientProvider { async state(): Promise { const entry = await McpAuth.get(this.mcpName) if (!entry?.oauthState) { - throw new Error(`No OAuth state saved for MCP server: ${this.mcpName}`) + // Generate a new state for fresh auth flows instead of throwing. + // Without this, the MCP SDK's authInternal() throws a generic Error + // that prevents UnauthorizedError from surfacing, causing clients to + // show "failed" instead of "needs authentication". + const bytes = new Uint8Array(32) + crypto.getRandomValues(bytes) + const state = Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join("") + await this.saveState(state) + return state } return entry.oauthState } diff --git a/packages/opencode/test/mcp/oauth-state.test.ts b/packages/opencode/test/mcp/oauth-state.test.ts new file mode 100644 index 00000000000..bc06b07a4de --- /dev/null +++ b/packages/opencode/test/mcp/oauth-state.test.ts @@ -0,0 +1,155 @@ +import { test, expect, mock, beforeEach } from "bun:test" + +// Mock UnauthorizedError +class MockUnauthorizedError extends Error { + constructor(message?: string) { + super(message ?? "Unauthorized") + this.name = "UnauthorizedError" + } +} + +// Track what options were passed to each transport constructor +const transportCalls: Array<{ + type: "streamable" | "sse" + url: string + options: { authProvider?: unknown } +}> = [] + +// Mock the transport constructors - simulate a server that returns 401 +mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ + StreamableHTTPClientTransport: class MockStreamableHTTP { + constructor(url: URL, options?: { authProvider?: unknown }) { + transportCalls.push({ + type: "streamable", + url: url.toString(), + options: options ?? {}, + }) + } + async start() { + throw new MockUnauthorizedError() + } + }, +})) + +mock.module("@modelcontextprotocol/sdk/client/sse.js", () => ({ + SSEClientTransport: class MockSSE { + constructor(url: URL, options?: { authProvider?: unknown }) { + transportCalls.push({ + type: "sse", + url: url.toString(), + options: options ?? {}, + }) + } + async start() { + throw new MockUnauthorizedError() + } + }, +})) + +mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ + Client: class MockClient { + async connect(transport: { start: () => Promise }) { + await transport.start() + } + }, +})) + +mock.module("@modelcontextprotocol/sdk/client/auth.js", () => ({ + UnauthorizedError: MockUnauthorizedError, +})) + +beforeEach(() => { + transportCalls.length = 0 +}) + +// Import after mocking +const { McpOAuthProvider } = await import("../../src/mcp/oauth-provider") +const { McpAuth } = await import("../../src/mcp/auth") +const { MCP } = await import("../../src/mcp/index") +const { Instance } = await import("../../src/project/instance") +const { tmpdir } = await import("../fixture/fixture") + +test("state() generates and persists a new state when none exists", async () => { + const provider = new McpOAuthProvider( + "test-state-gen", + "https://example.com/mcp", + {}, + { onRedirect: async () => {} }, + ) + + // Should NOT throw — should generate a new state + const state = await provider.state() + + // Should be a 64-char hex string (32 bytes) + expect(state).toMatch(/^[0-9a-f]{64}$/) + + // Calling again should return the same persisted value + const state2 = await provider.state() + expect(state2).toBe(state) + + // Verify it's actually persisted in McpAuth + const entry = await McpAuth.get("test-state-gen") + expect(entry?.oauthState).toBe(state) + + // Clean up + await McpAuth.remove("test-state-gen") +}) + +test("state() returns existing state when one is saved", async () => { + const savedState = "abc123def456" + await McpAuth.updateOAuthState("test-state-existing", savedState) + + const provider = new McpOAuthProvider( + "test-state-existing", + "https://example.com/mcp", + {}, + { onRedirect: async () => {} }, + ) + + const state = await provider.state() + expect(state).toBe(savedState) + + // Clean up + await McpAuth.remove("test-state-existing") +}) + +test("first unauthenticated connect results in needs_auth, not failed", async () => { + await using tmp = await tmpdir({ + init: async (dir) => { + await Bun.write( + `${dir}/opencode.json`, + JSON.stringify({ + $schema: "https://opencode.ai/config.json", + mcp: { + "test-needs-auth": { + type: "remote", + url: "https://example.com/mcp", + }, + }, + }), + ) + }, + }) + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const result = await MCP.add("test-needs-auth", { + type: "remote", + url: "https://example.com/mcp", + }) + + // The critical assertion: status should be "needs_auth" not "failed" + // Before the fix, state() would throw a generic Error which bypassed + // the UnauthorizedError check and fell through to "failed" + const status = result.status + if (typeof status === "object" && "status" in status && typeof status.status === "string") { + expect(status.status).toBe("needs_auth") + } else { + // status is a record of statuses keyed by name + const serverStatus = (status as Record)["test-needs-auth"] + expect(serverStatus?.status).toBe("needs_auth") + } + }, + }) +})