From 2622049fcf2d9ee9e7e4db61e1865519df6e4ff4 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Wed, 24 Jun 2026 16:24:37 -0500 Subject: [PATCH 1/8] fix(mcp): reset tokens before reauthentication --- packages/opencode/src/mcp/auth.ts | 20 +++++++ packages/opencode/src/mcp/index.ts | 6 ++ packages/opencode/src/mcp/oauth-provider.ts | 11 +--- packages/opencode/test/mcp/auth.test.ts | 57 +++++++++++++++++++ .../test/mcp/oauth-auto-connect.test.ts | 47 +++++++++++++++ 5 files changed, 132 insertions(+), 9 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index be03760c52b4..e32f7780a056 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -44,12 +44,15 @@ export interface Interface { readonly set: (mcpName: string, entry: Entry, serverUrl?: string) => Effect.Effect readonly remove: (mcpName: string) => Effect.Effect readonly updateTokens: (mcpName: string, tokens: Tokens, serverUrl?: string) => Effect.Effect + readonly clearTokens: (mcpName: string) => Effect.Effect readonly updateClientInfo: (mcpName: string, clientInfo: ClientInfo, serverUrl?: string) => Effect.Effect + readonly clearClientInfo: (mcpName: string) => Effect.Effect readonly updateCodeVerifier: (mcpName: string, codeVerifier: string) => Effect.Effect readonly clearCodeVerifier: (mcpName: string) => Effect.Effect readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect readonly getOAuthState: (mcpName: string) => Effect.Effect readonly clearOAuthState: (mcpName: string) => Effect.Effect + readonly resetForReauthentication: (mcpName: string) => Effect.Effect readonly isTokenExpired: (mcpName: string) => Effect.Effect } @@ -134,9 +137,23 @@ export const layer = Layer.effect( const updateClientInfo = updateField("clientInfo", "updateClientInfo") const updateCodeVerifier = updateField("codeVerifier", "updateCodeVerifier") const updateOAuthState = updateField("oauthState", "updateOAuthState") + const clearTokens = clearField("tokens", "clearTokens") + const clearClientInfo = clearField("clientInfo", "clearClientInfo") const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier") const clearOAuthState = clearField("oauthState", "clearOAuthState") + const resetForReauthentication = Effect.fn("McpAuth.resetForReauthentication")(function* (mcpName: string) { + yield* mutate((data) => { + const current = data[mcpName] + if (!current) return undefined + const entry = { ...current } + delete entry.tokens + delete entry.codeVerifier + delete entry.oauthState + return { ...data, [mcpName]: entry } + }) + }) + const getOAuthState = Effect.fn("McpAuth.getOAuthState")(function* (mcpName: string) { const entry = yield* get(mcpName) return entry?.oauthState @@ -156,12 +173,15 @@ export const layer = Layer.effect( set, remove, updateTokens, + clearTokens, updateClientInfo, + clearClientInfo, updateCodeVerifier, clearCodeVerifier, updateOAuthState, getOAuthState, clearOAuthState, + resetForReauthentication, isTokenExpired, }) }), diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index db673244b3d7..56a97ef2f7fd 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -800,6 +800,12 @@ export const layer = Layer.effect( const url = remoteURL(mcpConfig.url) if (!url) throw new Error(`Invalid MCP URL for "${mcpName}"`) + const pendingTransport = pendingOAuthTransports.get(mcpName) + McpOAuthCallback.cancelPending(mcpName) + pendingOAuthTransports.delete(mcpName) + yield* Effect.tryPromise(() => pendingTransport?.close() ?? Promise.resolve()).pipe(Effect.ignore) + yield* auth.resetForReauthentication(mcpName) + // OAuth config is optional - if not provided, we'll use auto-discovery const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index aa29777f5447..c4681433e42d 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -163,22 +163,15 @@ export class McpOAuthProvider implements OAuthClientProvider { } async invalidateCredentials(type: "all" | "client" | "tokens"): Promise { - const entry = await Effect.runPromise(this.auth.get(this.mcpName)) - if (!entry) { - return - } - switch (type) { case "all": await Effect.runPromise(this.auth.remove(this.mcpName)) break case "client": - delete entry.clientInfo - await Effect.runPromise(this.auth.set(this.mcpName, entry)) + await Effect.runPromise(this.auth.clearClientInfo(this.mcpName)) break case "tokens": - delete entry.tokens - await Effect.runPromise(this.auth.set(this.mcpName, entry)) + await Effect.runPromise(this.auth.clearTokens(this.mcpName)) break } } diff --git a/packages/opencode/test/mcp/auth.test.ts b/packages/opencode/test/mcp/auth.test.ts index 0fdfe78b2af9..53e2e007263a 100644 --- a/packages/opencode/test/mcp/auth.test.ts +++ b/packages/opencode/test/mcp/auth.test.ts @@ -4,6 +4,7 @@ import { Effect, Layer } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { McpAuth } from "../../src/mcp/auth" +import { McpOAuthProvider } from "../../src/mcp/oauth-provider" function authFile() { let raw = "" @@ -76,3 +77,59 @@ test("serializes concurrent auth file updates across service instances", async ( }), ) }) + +test("concurrent token invalidation does not overwrite newer client registration", async () => { + const name = `token-invalidation-${crypto.randomUUID()}` + const url = "https://mcp.example.com/exact/path" + + await Effect.runPromise( + Effect.gen(function* () { + const first = yield* McpAuth.Service + const second = yield* authService(FSUtil.defaultLayer) + const provider = new McpOAuthProvider(name, url, {}, { onRedirect: async () => {} }, first) + + yield* first.updateTokens(name, { accessToken: "old-token" }, url) + yield* Effect.all( + [ + Effect.promise(() => provider.invalidateCredentials("tokens")), + second.updateClientInfo(name, { clientId: "new-client" }, url), + ], + { concurrency: "unbounded" }, + ) + + const entry = yield* first.get(name) + expect(entry?.tokens).toBeUndefined() + expect(entry?.clientInfo?.clientId).toBe("new-client") + expect(entry?.serverUrl).toBe(url) + yield* first.remove(name) + }).pipe(Effect.provide(McpAuth.defaultLayer)), + ) +}) + +test("concurrent client invalidation does not overwrite newer tokens", async () => { + const name = `client-invalidation-${crypto.randomUUID()}` + const url = "https://mcp.example.com/exact/path" + + await Effect.runPromise( + Effect.gen(function* () { + const first = yield* McpAuth.Service + const second = yield* authService(FSUtil.defaultLayer) + const provider = new McpOAuthProvider(name, url, {}, { onRedirect: async () => {} }, first) + + yield* first.updateClientInfo(name, { clientId: "old-client" }, url) + yield* Effect.all( + [ + Effect.promise(() => provider.invalidateCredentials("client")), + second.updateTokens(name, { accessToken: "new-token" }, url), + ], + { concurrency: "unbounded" }, + ) + + const entry = yield* first.get(name) + expect(entry?.clientInfo).toBeUndefined() + expect(entry?.tokens?.accessToken).toBe("new-token") + expect(entry?.serverUrl).toBe(url) + yield* first.remove(name) + }).pipe(Effect.provide(McpAuth.defaultLayer)), + ) +}) diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index 480792b1cb56..bc9fb3b8aae5 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -23,6 +23,7 @@ let simulateAuthFlow = true let connectSucceedsImmediately = false let serverCapabilities: { tools?: object; resources?: object } = { tools: {} } let listToolsCalls = 0 +let transportCloseCount = 0 // Mock the transport constructors to simulate OAuth auto-auth on 401 void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ @@ -32,6 +33,7 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ state?: () => Promise redirectToAuthorization?: (url: URL) => Promise saveCodeVerifier?: (v: string) => Promise + tokens?: () => Promise<{ access_token: string } | undefined> } | undefined constructor(url: URL, options?: { authProvider?: unknown }) { @@ -49,6 +51,7 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ // It calls auth() which eventually calls provider.state(), then // provider.redirectToAuthorization(), then throws UnauthorizedError. if (simulateAuthFlow && this.authProvider) { + if (await this.authProvider.tokens?.()) throw new MockUnauthorizedError() // The SDK calls provider.state() to get the OAuth state parameter if (this.authProvider.state) { await this.authProvider.state() @@ -66,6 +69,9 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ throw new MockUnauthorizedError() } async finishAuth(_code: string) {} + async close() { + transportCloseCount++ + } }, })) @@ -125,6 +131,7 @@ beforeEach(() => { connectSucceedsImmediately = false serverCapabilities = { tools: {} } listToolsCalls = 0 + transportCloseCount = 0 }) // Import modules after mocking @@ -133,6 +140,7 @@ const { EventV2Bridge } = await import("../../src/event-v2-bridge") const { Config } = await import("../../src/config/config") const { McpAuth } = await import("../../src/mcp/auth") const { McpOAuthProvider } = await import("../../src/mcp/oauth-provider") +const { McpOAuthCallback } = await import("../../src/mcp/oauth-callback") const { FSUtil } = await import("@opencode-ai/core/fs-util") const { CrossSpawnSpawner } = await import("@opencode-ai/core/cross-spawn-spawner") @@ -227,6 +235,45 @@ mcpTest.instance("state() returns existing state when one is saved", () => }), ) +mcpTest.instance( + "reauthentication starts fresh while preserving dynamic client registration", + () => + Effect.gen(function* () { + yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) + const mcp = yield* MCP.Service + const auth = yield* McpAuth.Service + const name = "test-reauth" + const url = "https://example.com/mcp" + const clientInfo = { clientId: "dynamic-client", clientSecret: "dynamic-secret" } + + yield* auth.updateClientInfo(name, clientInfo, url) + yield* auth.updateTokens(name, { accessToken: "stale-token" }, url) + yield* auth.updateCodeVerifier(name, "stale-verifier") + yield* auth.updateOAuthState(name, "stale-state") + + const first = yield* mcp.startAuth(name) + const afterFirst = yield* auth.get(name) + + expect(first.authorizationUrl).toContain("https://auth.example.com/authorize") + expect(first.oauthState).not.toBe("stale-state") + expect(afterFirst?.tokens).toBeUndefined() + expect(afterFirst?.clientInfo).toEqual(clientInfo) + expect(afterFirst?.serverUrl).toBe(url) + expect(afterFirst?.codeVerifier).toBe("test-verifier") + expect(afterFirst?.oauthState).toBe(first.oauthState) + + const cancelled = McpOAuthCallback.waitForCallback(first.oauthState, name).catch((error) => error) + const closedBeforeRestart = transportCloseCount + yield* mcp.startAuth(name) + expect(transportCloseCount).toBe(closedBeforeRestart + 1) + const cancellation = yield* Effect.promise(() => cancelled) + expect(cancellation).toBeInstanceOf(Error) + if (!(cancellation instanceof Error)) throw new Error("Expected abandoned authorization to be cancelled") + expect(cancellation.message).toBe("Authorization cancelled") + }), + { config: config("test-reauth") }, +) + mcpTest.instance( "authenticate() stores a connected client when auth completes without redirect", () => From 6d6d939b1227d9fab43642996048d24bafcea9b1 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Wed, 24 Jun 2026 17:04:50 -0500 Subject: [PATCH 2/8] test(mcp): make credential races deterministic --- packages/opencode/test/mcp/auth.test.ts | 60 ++++++++++++++++++------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/packages/opencode/test/mcp/auth.test.ts b/packages/opencode/test/mcp/auth.test.ts index 53e2e007263a..4593fad2600a 100644 --- a/packages/opencode/test/mcp/auth.test.ts +++ b/packages/opencode/test/mcp/auth.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "bun:test" import { setTimeout as sleep } from "node:timers/promises" -import { Effect, Layer } from "effect" +import { Deferred, Effect, Fiber, Layer } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { McpAuth } from "../../src/mcp/auth" @@ -53,6 +53,20 @@ function authService(layer: Layer.Layer) { ) } +function gateCredentialInvalidation( + auth: McpAuth.Interface, + ready: Deferred.Deferred, + release: Deferred.Deferred, +) { + const gate = Deferred.succeed(ready, undefined).pipe(Effect.ignore, Effect.andThen(Deferred.await(release))) + return McpAuth.Service.of({ + ...auth, + get: (mcpName) => auth.get(mcpName).pipe(Effect.tap(() => gate)), + clearTokens: (mcpName) => gate.pipe(Effect.andThen(auth.clearTokens(mcpName))), + clearClientInfo: (mcpName) => gate.pipe(Effect.andThen(auth.clearClientInfo(mcpName))), + }) +} + test("serializes concurrent auth file updates across service instances", async () => { const file = authFile() @@ -86,16 +100,22 @@ test("concurrent token invalidation does not overwrite newer client registration Effect.gen(function* () { const first = yield* McpAuth.Service const second = yield* authService(FSUtil.defaultLayer) - const provider = new McpOAuthProvider(name, url, {}, { onRedirect: async () => {} }, first) + const ready = yield* Deferred.make() + const release = yield* Deferred.make() + const provider = new McpOAuthProvider( + name, + url, + {}, + { onRedirect: async () => {} }, + gateCredentialInvalidation(first, ready, release), + ) yield* first.updateTokens(name, { accessToken: "old-token" }, url) - yield* Effect.all( - [ - Effect.promise(() => provider.invalidateCredentials("tokens")), - second.updateClientInfo(name, { clientId: "new-client" }, url), - ], - { concurrency: "unbounded" }, - ) + const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("tokens")).pipe(Effect.forkChild) + yield* Deferred.await(ready) + yield* second.updateClientInfo(name, { clientId: "new-client" }, url) + yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) + yield* Fiber.join(invalidation) const entry = yield* first.get(name) expect(entry?.tokens).toBeUndefined() @@ -114,16 +134,22 @@ test("concurrent client invalidation does not overwrite newer tokens", async () Effect.gen(function* () { const first = yield* McpAuth.Service const second = yield* authService(FSUtil.defaultLayer) - const provider = new McpOAuthProvider(name, url, {}, { onRedirect: async () => {} }, first) + const ready = yield* Deferred.make() + const release = yield* Deferred.make() + const provider = new McpOAuthProvider( + name, + url, + {}, + { onRedirect: async () => {} }, + gateCredentialInvalidation(first, ready, release), + ) yield* first.updateClientInfo(name, { clientId: "old-client" }, url) - yield* Effect.all( - [ - Effect.promise(() => provider.invalidateCredentials("client")), - second.updateTokens(name, { accessToken: "new-token" }, url), - ], - { concurrency: "unbounded" }, - ) + const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("client")).pipe(Effect.forkChild) + yield* Deferred.await(ready) + yield* second.updateTokens(name, { accessToken: "new-token" }, url) + yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) + yield* Fiber.join(invalidation) const entry = yield* first.get(name) expect(entry?.clientInfo).toBeUndefined() From 41726e7d8641e06eddc94887d94e9649dee6bfc9 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Thu, 25 Jun 2026 23:06:17 -0500 Subject: [PATCH 3/8] fix(mcp): renew dynamic client on reauthentication --- packages/opencode/src/mcp/auth.ts | 1 + packages/opencode/test/mcp/oauth-auto-connect.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index b2a6d87c1677..4fbc703295af 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -147,6 +147,7 @@ export const layer = Layer.effect( if (!current) return undefined const entry = { ...current } delete entry.tokens + delete entry.clientInfo delete entry.codeVerifier delete entry.oauthState return { ...data, [mcpName]: entry } diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index 19cc030fb3e4..fd70b9ffffe0 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -236,7 +236,7 @@ mcpTest.instance("state() returns existing state when one is saved", () => ) mcpTest.instance( - "reauthentication starts fresh while preserving dynamic client registration", + "reauthentication starts fresh with a new dynamic client registration", () => Effect.gen(function* () { yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) @@ -257,7 +257,7 @@ mcpTest.instance( expect(first.authorizationUrl).toContain("https://auth.example.com/authorize") expect(first.oauthState).not.toBe("stale-state") expect(afterFirst?.tokens).toBeUndefined() - expect(afterFirst?.clientInfo).toEqual(clientInfo) + expect(afterFirst?.clientInfo).toBeUndefined() expect(afterFirst?.serverUrl).toBe(url) expect(afterFirst?.codeVerifier).toBe("test-verifier") expect(afterFirst?.oauthState).toBe(first.oauthState) From b9fe4d69db1080a5e5b0756662a117a638a91707 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Thu, 25 Jun 2026 23:15:07 -0500 Subject: [PATCH 4/8] fix(mcp): defer reauthentication persistence --- packages/opencode/src/mcp/auth.ts | 8 +- packages/opencode/src/mcp/index.ts | 27 ++++--- packages/opencode/src/mcp/oauth-provider.ts | 47 ++++++++++++ .../test/mcp/oauth-auto-connect.test.ts | 75 ++++++++++++++++++- 4 files changed, 136 insertions(+), 21 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index 4fbc703295af..137813eae5d9 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -52,7 +52,7 @@ export interface Interface { readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect readonly getOAuthState: (mcpName: string) => Effect.Effect readonly clearOAuthState: (mcpName: string) => Effect.Effect - readonly resetForReauthentication: (mcpName: string) => Effect.Effect + readonly resetOAuthFlow: (mcpName: string) => Effect.Effect } export class Service extends Context.Service()("@opencode/McpAuth") {} @@ -141,13 +141,11 @@ export const layer = Layer.effect( const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier") const clearOAuthState = clearField("oauthState", "clearOAuthState") - const resetForReauthentication = Effect.fn("McpAuth.resetForReauthentication")(function* (mcpName: string) { + const resetOAuthFlow = Effect.fn("McpAuth.resetOAuthFlow")(function* (mcpName: string) { yield* mutate((data) => { const current = data[mcpName] if (!current) return undefined const entry = { ...current } - delete entry.tokens - delete entry.clientInfo delete entry.codeVerifier delete entry.oauthState return { ...data, [mcpName]: entry } @@ -174,7 +172,7 @@ export const layer = Layer.effect( updateOAuthState, getOAuthState, clearOAuthState, - resetForReauthentication, + resetOAuthFlow, }) }), ) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index e8b0ed64c721..7941e7a44f2b 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -109,7 +109,7 @@ export type Status = Schema.Schema.Type // Store transports for OAuth servers to allow finishing auth type TransportWithAuth = StreamableHTTPClientTransport | SSEClientTransport -const pendingOAuthTransports = new Map() +const pendingOAuthTransports = new Map() // Prompt cache types type PromptInfo = Awaited>["prompts"][number] @@ -301,7 +301,7 @@ export const layer = Layer.effect( }) .pipe(Effect.ignore, Effect.as(undefined)) } else { - pendingOAuthTransports.set(key, transport) + pendingOAuthTransports.set(key, { transport, provider: authProvider }) lastStatus = { status: "needs_auth" as const } return events .publish(TuiEvent.ToastShow, { @@ -803,11 +803,11 @@ export const layer = Layer.effect( const url = remoteURL(mcpConfig.url) if (!url) throw new Error(`Invalid MCP URL for "${mcpName}"`) - const pendingTransport = pendingOAuthTransports.get(mcpName) + const pending = pendingOAuthTransports.get(mcpName) McpOAuthCallback.cancelPending(mcpName) pendingOAuthTransports.delete(mcpName) - yield* Effect.tryPromise(() => pendingTransport?.close() ?? Promise.resolve()).pipe(Effect.ignore) - yield* auth.resetForReauthentication(mcpName) + yield* Effect.tryPromise(() => pending?.transport.close() ?? Promise.resolve()).pipe(Effect.ignore) + yield* auth.resetOAuthFlow(mcpName) // OAuth config is optional - if not provided, we'll use auto-discovery const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined @@ -833,6 +833,7 @@ export const layer = Layer.effect( clientSecret: oauthConfig?.clientSecret, scope: oauthConfig?.scope, redirectUri: effectiveRedirectUri, + deferPersistence: true, }, { onRedirect: async (url) => { @@ -851,15 +852,16 @@ export const layer = Layer.effect( return yield* Effect.tryPromise({ try: () => { const client = createClient(directory) - return client - .connect(transport) - .then(() => ({ authorizationUrl: "", oauthState, client }) satisfies AuthResult) + return client.connect(transport).then(async () => { + await authProvider.commit() + return { authorizationUrl: "", oauthState, client } satisfies AuthResult + }) }, catch: (error) => error, }).pipe( Effect.catch((error) => { if (error instanceof UnauthorizedError && capturedUrl) { - pendingOAuthTransports.set(mcpName, transport) + pendingOAuthTransports.set(mcpName, { transport, provider: authProvider }) return Effect.succeed({ authorizationUrl: capturedUrl.toString(), oauthState } satisfies AuthResult) } return Effect.die(error) @@ -930,11 +932,11 @@ export const layer = Layer.effect( const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { yield* requireMcpConfig(mcpName) - const transport = pendingOAuthTransports.get(mcpName) - if (!transport) throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) + const pending = pendingOAuthTransports.get(mcpName) + if (!pending) throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) const result = yield* Effect.tryPromise({ - try: () => transport.finishAuth(authorizationCode).then(() => true as const), + try: () => pending.transport.finishAuth(authorizationCode).then(() => true as const), catch: (error) => { return error }, @@ -944,6 +946,7 @@ export const layer = Layer.effect( return { status: "failed", error: "OAuth completion failed" } satisfies Status } + yield* Effect.promise(() => pending.provider?.commit() ?? Promise.resolve()) yield* auth.clearCodeVerifier(mcpName) pendingOAuthTransports.delete(mcpName) diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index c4681433e42d..faec47297943 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -17,6 +17,7 @@ export interface McpOAuthConfig { scope?: string callbackPort?: number redirectUri?: string + deferPersistence?: boolean } export interface McpOAuthCallbacks { @@ -24,6 +25,9 @@ export interface McpOAuthCallbacks { } export class McpOAuthProvider implements OAuthClientProvider { + private pendingClientInfo?: OAuthClientInformationFull + private pendingTokens?: OAuthTokens + constructor( private mcpName: string, private serverUrl: string, @@ -61,6 +65,8 @@ export class McpOAuthProvider implements OAuthClientProvider { } } + if (this.config.deferPersistence) return this.pendingClientInfo + // Check stored client info (from dynamic registration) // Use getForUrl to validate credentials are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) @@ -80,6 +86,10 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveClientInformation(info: OAuthClientInformationFull): Promise { + if (this.config.deferPersistence) { + this.pendingClientInfo = info + return + } await Effect.runPromise( this.auth.updateClientInfo( this.mcpName, @@ -95,6 +105,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async tokens(): Promise { + if (this.config.deferPersistence) return this.pendingTokens // Use getForUrl to validate tokens are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (!entry?.tokens) return undefined @@ -111,6 +122,10 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { + if (this.config.deferPersistence) { + this.pendingTokens = tokens + return + } await Effect.runPromise( this.auth.updateTokens( this.mcpName, @@ -163,6 +178,11 @@ export class McpOAuthProvider implements OAuthClientProvider { } async invalidateCredentials(type: "all" | "client" | "tokens"): Promise { + if (this.config.deferPersistence) { + if (type === "all" || type === "client") this.pendingClientInfo = undefined + if (type === "all" || type === "tokens") this.pendingTokens = undefined + return + } switch (type) { case "all": await Effect.runPromise(this.auth.remove(this.mcpName)) @@ -175,6 +195,33 @@ export class McpOAuthProvider implements OAuthClientProvider { break } } + + async commit(): Promise { + if (!this.config.deferPersistence || !this.pendingTokens) return + await Effect.runPromise( + this.auth.set( + this.mcpName, + { + tokens: { + accessToken: this.pendingTokens.access_token, + refreshToken: this.pendingTokens.refresh_token, + expiresAt: this.pendingTokens.expires_in ? Date.now() / 1000 + this.pendingTokens.expires_in : undefined, + scope: this.pendingTokens.scope, + }, + clientInfo: + this.pendingClientInfo && !this.config.clientId + ? { + clientId: this.pendingClientInfo.client_id, + clientSecret: this.pendingClientInfo.client_secret, + clientIdIssuedAt: this.pendingClientInfo.client_id_issued_at, + clientSecretExpiresAt: this.pendingClientInfo.client_secret_expires_at, + } + : undefined, + }, + this.serverUrl, + ), + ) + } } export { OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH } diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index fd70b9ffffe0..7e8f26af92f6 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -24,6 +24,8 @@ let connectSucceedsImmediately = false let serverCapabilities: { tools?: object; resources?: object } = { tools: {} } let listToolsCalls = 0 let transportCloseCount = 0 +let finishAuthFails = false +let finishAuthStoresCredentials = false // Mock the transport constructors to simulate OAuth auto-auth on 401 void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ @@ -34,6 +36,8 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ redirectToAuthorization?: (url: URL) => Promise saveCodeVerifier?: (v: string) => Promise tokens?: () => Promise<{ access_token: string } | undefined> + saveClientInformation?: (info: { client_id: string; client_secret?: string }) => Promise + saveTokens?: (tokens: { access_token: string; token_type: string }) => Promise } | undefined constructor(url: URL, options?: { authProvider?: unknown }) { @@ -68,7 +72,13 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ } throw new MockUnauthorizedError() } - async finishAuth(_code: string) {} + async finishAuth(_code: string) { + if (finishAuthFails) throw new Error("Token exchange failed") + if (finishAuthStoresCredentials) { + await this.authProvider?.saveClientInformation?.({ client_id: "replacement-client" }) + await this.authProvider?.saveTokens?.({ access_token: "replacement-token", token_type: "Bearer" }) + } + } async close() { transportCloseCount++ } @@ -132,6 +142,8 @@ beforeEach(() => { serverCapabilities = { tools: {} } listToolsCalls = 0 transportCloseCount = 0 + finishAuthFails = false + finishAuthStoresCredentials = false }) // Import modules after mocking @@ -236,7 +248,7 @@ mcpTest.instance("state() returns existing state when one is saved", () => ) mcpTest.instance( - "reauthentication starts fresh with a new dynamic client registration", + "reauthentication preserves existing credentials until replacement succeeds", () => Effect.gen(function* () { yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) @@ -256,8 +268,8 @@ mcpTest.instance( expect(first.authorizationUrl).toContain("https://auth.example.com/authorize") expect(first.oauthState).not.toBe("stale-state") - expect(afterFirst?.tokens).toBeUndefined() - expect(afterFirst?.clientInfo).toBeUndefined() + expect(afterFirst?.tokens?.accessToken).toBe("stale-token") + expect(afterFirst?.clientInfo).toEqual(clientInfo) expect(afterFirst?.serverUrl).toBe(url) expect(afterFirst?.codeVerifier).toBe("test-verifier") expect(afterFirst?.oauthState).toBe(first.oauthState) @@ -270,10 +282,65 @@ mcpTest.instance( expect(cancellation).toBeInstanceOf(Error) if (!(cancellation instanceof Error)) throw new Error("Expected abandoned authorization to be cancelled") expect(cancellation.message).toBe("Authorization cancelled") + const afterCancellation = yield* auth.get(name) + expect(afterCancellation?.tokens?.accessToken).toBe("stale-token") + expect(afterCancellation?.clientInfo).toEqual(clientInfo) }), { config: config("test-reauth") }, ) +mcpTest.instance( + "failed reauthentication preserves existing credentials", + () => + Effect.gen(function* () { + yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) + const mcp = yield* MCP.Service + const auth = yield* McpAuth.Service + const name = "test-reauth-failure" + const url = "https://example.com/mcp" + const clientInfo = { clientId: "dynamic-client", clientSecret: "dynamic-secret" } + + yield* auth.updateClientInfo(name, clientInfo, url) + yield* auth.updateTokens(name, { accessToken: "working-token" }, url) + yield* mcp.startAuth(name) + finishAuthFails = true + + expect(yield* mcp.finishAuth(name, "invalid-code")).toEqual({ + status: "failed", + error: "OAuth completion failed", + }) + const entry = yield* auth.get(name) + expect(entry?.tokens?.accessToken).toBe("working-token") + expect(entry?.clientInfo).toEqual(clientInfo) + }), + { config: config("test-reauth-failure") }, +) + +mcpTest.instance( + "successful reauthentication commits replacement credentials", + () => + Effect.gen(function* () { + yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) + const mcp = yield* MCP.Service + const auth = yield* McpAuth.Service + const name = "test-reauth-success" + const url = "https://example.com/mcp" + + yield* auth.updateClientInfo(name, { clientId: "old-client" }, url) + yield* auth.updateTokens(name, { accessToken: "old-token" }, url) + yield* mcp.startAuth(name) + finishAuthStoresCredentials = true + connectSucceedsImmediately = true + + expect((yield* mcp.finishAuth(name, "valid-code")).status).toBe("connected") + const entry = yield* auth.get(name) + expect(entry?.tokens?.accessToken).toBe("replacement-token") + expect(entry?.clientInfo?.clientId).toBe("replacement-client") + expect(entry?.serverUrl).toBe(url) + }), + { config: config("test-reauth-success") }, +) + mcpTest.instance( "auth status only reports credentials stored for the configured server URL", () => From 2e33986cfdb2ad4ed819351b3fb202bab9805b73 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Thu, 25 Jun 2026 23:29:58 -0500 Subject: [PATCH 5/8] fix(mcp): guard credential invalidation races --- packages/opencode/src/mcp/auth.ts | 34 +++++++++--- packages/opencode/src/mcp/oauth-provider.ts | 59 +++++++++------------ packages/opencode/test/mcp/auth.test.ts | 21 ++++---- 3 files changed, 62 insertions(+), 52 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index 137813eae5d9..766cb34b98a0 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -5,6 +5,7 @@ import { Global } from "@opencode-ai/core/global" import { Effect, Layer, Context, Option, Schema } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" +import { isDeepEqual } from "remeda" export const Tokens = Schema.Struct({ accessToken: Schema.mutableKey(Schema.String), @@ -44,9 +45,12 @@ export interface Interface { readonly set: (mcpName: string, entry: Entry, serverUrl?: string) => Effect.Effect readonly remove: (mcpName: string) => Effect.Effect readonly updateTokens: (mcpName: string, tokens: Tokens, serverUrl?: string) => Effect.Effect - readonly clearTokens: (mcpName: string) => Effect.Effect readonly updateClientInfo: (mcpName: string, clientInfo: ClientInfo, serverUrl?: string) => Effect.Effect - readonly clearClientInfo: (mcpName: string) => Effect.Effect + readonly invalidateCredentials: ( + mcpName: string, + type: "all" | "client" | "tokens", + expected: Pick, + ) => Effect.Effect readonly updateCodeVerifier: (mcpName: string, codeVerifier: string) => Effect.Effect readonly clearCodeVerifier: (mcpName: string) => Effect.Effect readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect @@ -136,11 +140,26 @@ export const layer = Layer.effect( const updateClientInfo = updateField("clientInfo", "updateClientInfo") const updateCodeVerifier = updateField("codeVerifier", "updateCodeVerifier") const updateOAuthState = updateField("oauthState", "updateOAuthState") - const clearTokens = clearField("tokens", "clearTokens") - const clearClientInfo = clearField("clientInfo", "clearClientInfo") const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier") const clearOAuthState = clearField("oauthState", "clearOAuthState") + const invalidateCredentials = Effect.fn("McpAuth.invalidateCredentials")(function* ( + mcpName: string, + type: "all" | "client" | "tokens", + expected: Pick, + ) { + yield* mutate((data) => { + const current = data[mcpName] + if (!current) return undefined + const entry = { ...current } + if ((type === "all" || type === "tokens") && credentialsEqual(current.tokens, expected.tokens)) + delete entry.tokens + if ((type === "all" || type === "client") && credentialsEqual(current.clientInfo, expected.clientInfo)) + delete entry.clientInfo + return { ...data, [mcpName]: entry } + }) + }) + const resetOAuthFlow = Effect.fn("McpAuth.resetOAuthFlow")(function* (mcpName: string) { yield* mutate((data) => { const current = data[mcpName] @@ -164,9 +183,8 @@ export const layer = Layer.effect( set, remove, updateTokens, - clearTokens, updateClientInfo, - clearClientInfo, + invalidateCredentials, updateCodeVerifier, clearCodeVerifier, updateOAuthState, @@ -181,4 +199,8 @@ export const defaultLayer = layer.pipe(Layer.provide(EffectFlock.defaultLayer), export const node = LayerNode.make({ service: Service, layer: layer, deps: [FSUtil.node, EffectFlock.node] }) +function credentialsEqual(left: Tokens | ClientInfo | undefined, right: Tokens | ClientInfo | undefined) { + return isDeepEqual(left, right) +} + export * as McpAuth from "./auth" diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index faec47297943..ce8b3a8ef73a 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -27,6 +27,8 @@ export interface McpOAuthCallbacks { export class McpOAuthProvider implements OAuthClientProvider { private pendingClientInfo?: OAuthClientInformationFull private pendingTokens?: OAuthTokens + private observedClientInfo?: McpAuth.ClientInfo + private observedTokens?: McpAuth.Tokens constructor( private mcpName: string, @@ -71,6 +73,7 @@ export class McpOAuthProvider implements OAuthClientProvider { // Use getForUrl to validate credentials are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (entry?.clientInfo) { + this.observedClientInfo = entry.clientInfo // Check if client secret has expired if (entry.clientInfo.clientSecretExpiresAt && entry.clientInfo.clientSecretExpiresAt < Date.now() / 1000) { return undefined @@ -90,18 +93,13 @@ export class McpOAuthProvider implements OAuthClientProvider { this.pendingClientInfo = info return } - await Effect.runPromise( - this.auth.updateClientInfo( - this.mcpName, - { - clientId: info.client_id, - clientSecret: info.client_secret, - clientIdIssuedAt: info.client_id_issued_at, - clientSecretExpiresAt: info.client_secret_expires_at, - }, - this.serverUrl, - ), - ) + this.observedClientInfo = { + clientId: info.client_id, + clientSecret: info.client_secret, + clientIdIssuedAt: info.client_id_issued_at, + clientSecretExpiresAt: info.client_secret_expires_at, + } + await Effect.runPromise(this.auth.updateClientInfo(this.mcpName, this.observedClientInfo, this.serverUrl)) } async tokens(): Promise { @@ -109,6 +107,7 @@ export class McpOAuthProvider implements OAuthClientProvider { // Use getForUrl to validate tokens are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (!entry?.tokens) return undefined + this.observedTokens = entry.tokens return { access_token: entry.tokens.accessToken, @@ -126,18 +125,13 @@ export class McpOAuthProvider implements OAuthClientProvider { this.pendingTokens = tokens return } - await Effect.runPromise( - this.auth.updateTokens( - this.mcpName, - { - accessToken: tokens.access_token, - refreshToken: tokens.refresh_token, - expiresAt: tokens.expires_in ? Date.now() / 1000 + tokens.expires_in : undefined, - scope: tokens.scope, - }, - this.serverUrl, - ), - ) + this.observedTokens = { + accessToken: tokens.access_token, + refreshToken: tokens.refresh_token, + expiresAt: tokens.expires_in ? Date.now() / 1000 + tokens.expires_in : undefined, + scope: tokens.scope, + } + await Effect.runPromise(this.auth.updateTokens(this.mcpName, this.observedTokens, this.serverUrl)) } async redirectToAuthorization(authorizationUrl: URL): Promise { @@ -183,17 +177,12 @@ export class McpOAuthProvider implements OAuthClientProvider { if (type === "all" || type === "tokens") this.pendingTokens = undefined return } - switch (type) { - case "all": - await Effect.runPromise(this.auth.remove(this.mcpName)) - break - case "client": - await Effect.runPromise(this.auth.clearClientInfo(this.mcpName)) - break - case "tokens": - await Effect.runPromise(this.auth.clearTokens(this.mcpName)) - break - } + await Effect.runPromise( + this.auth.invalidateCredentials(this.mcpName, type, { + clientInfo: this.observedClientInfo, + tokens: this.observedTokens, + }), + ) } async commit(): Promise { diff --git a/packages/opencode/test/mcp/auth.test.ts b/packages/opencode/test/mcp/auth.test.ts index 4593fad2600a..9fa77725bf0d 100644 --- a/packages/opencode/test/mcp/auth.test.ts +++ b/packages/opencode/test/mcp/auth.test.ts @@ -61,9 +61,8 @@ function gateCredentialInvalidation( const gate = Deferred.succeed(ready, undefined).pipe(Effect.ignore, Effect.andThen(Deferred.await(release))) return McpAuth.Service.of({ ...auth, - get: (mcpName) => auth.get(mcpName).pipe(Effect.tap(() => gate)), - clearTokens: (mcpName) => gate.pipe(Effect.andThen(auth.clearTokens(mcpName))), - clearClientInfo: (mcpName) => gate.pipe(Effect.andThen(auth.clearClientInfo(mcpName))), + invalidateCredentials: (mcpName, type, expected) => + gate.pipe(Effect.andThen(auth.invalidateCredentials(mcpName, type, expected))), }) } @@ -92,7 +91,7 @@ test("serializes concurrent auth file updates across service instances", async ( ) }) -test("concurrent token invalidation does not overwrite newer client registration", async () => { +test("stale token invalidation does not delete newer tokens", async () => { const name = `token-invalidation-${crypto.randomUUID()}` const url = "https://mcp.example.com/exact/path" @@ -111,22 +110,22 @@ test("concurrent token invalidation does not overwrite newer client registration ) yield* first.updateTokens(name, { accessToken: "old-token" }, url) + yield* Effect.promise(() => provider.tokens()) const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("tokens")).pipe(Effect.forkChild) yield* Deferred.await(ready) - yield* second.updateClientInfo(name, { clientId: "new-client" }, url) + yield* second.updateTokens(name, { accessToken: "new-token" }, url) yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) yield* Fiber.join(invalidation) const entry = yield* first.get(name) - expect(entry?.tokens).toBeUndefined() - expect(entry?.clientInfo?.clientId).toBe("new-client") + expect(entry?.tokens?.accessToken).toBe("new-token") expect(entry?.serverUrl).toBe(url) yield* first.remove(name) }).pipe(Effect.provide(McpAuth.defaultLayer)), ) }) -test("concurrent client invalidation does not overwrite newer tokens", async () => { +test("stale client invalidation does not delete newer registration", async () => { const name = `client-invalidation-${crypto.randomUUID()}` const url = "https://mcp.example.com/exact/path" @@ -145,15 +144,15 @@ test("concurrent client invalidation does not overwrite newer tokens", async () ) yield* first.updateClientInfo(name, { clientId: "old-client" }, url) + yield* Effect.promise(() => provider.clientInformation()) const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("client")).pipe(Effect.forkChild) yield* Deferred.await(ready) - yield* second.updateTokens(name, { accessToken: "new-token" }, url) + yield* second.updateClientInfo(name, { clientId: "new-client" }, url) yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) yield* Fiber.join(invalidation) const entry = yield* first.get(name) - expect(entry?.clientInfo).toBeUndefined() - expect(entry?.tokens?.accessToken).toBe("new-token") + expect(entry?.clientInfo?.clientId).toBe("new-client") expect(entry?.serverUrl).toBe(url) yield* first.remove(name) }).pipe(Effect.provide(McpAuth.defaultLayer)), From 8dc7b17d7b3534744a2c6853395cc9fa7ab38f79 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Thu, 25 Jun 2026 23:32:12 -0500 Subject: [PATCH 6/8] Revert "fix(mcp): guard credential invalidation races" This reverts commit 2e33986cfdb2ad4ed819351b3fb202bab9805b73. --- packages/opencode/src/mcp/auth.ts | 34 +++--------- packages/opencode/src/mcp/oauth-provider.ts | 59 ++++++++++++--------- packages/opencode/test/mcp/auth.test.ts | 21 ++++---- 3 files changed, 52 insertions(+), 62 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index 766cb34b98a0..137813eae5d9 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -5,7 +5,6 @@ import { Global } from "@opencode-ai/core/global" import { Effect, Layer, Context, Option, Schema } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" -import { isDeepEqual } from "remeda" export const Tokens = Schema.Struct({ accessToken: Schema.mutableKey(Schema.String), @@ -45,12 +44,9 @@ export interface Interface { readonly set: (mcpName: string, entry: Entry, serverUrl?: string) => Effect.Effect readonly remove: (mcpName: string) => Effect.Effect readonly updateTokens: (mcpName: string, tokens: Tokens, serverUrl?: string) => Effect.Effect + readonly clearTokens: (mcpName: string) => Effect.Effect readonly updateClientInfo: (mcpName: string, clientInfo: ClientInfo, serverUrl?: string) => Effect.Effect - readonly invalidateCredentials: ( - mcpName: string, - type: "all" | "client" | "tokens", - expected: Pick, - ) => Effect.Effect + readonly clearClientInfo: (mcpName: string) => Effect.Effect readonly updateCodeVerifier: (mcpName: string, codeVerifier: string) => Effect.Effect readonly clearCodeVerifier: (mcpName: string) => Effect.Effect readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect @@ -140,26 +136,11 @@ export const layer = Layer.effect( const updateClientInfo = updateField("clientInfo", "updateClientInfo") const updateCodeVerifier = updateField("codeVerifier", "updateCodeVerifier") const updateOAuthState = updateField("oauthState", "updateOAuthState") + const clearTokens = clearField("tokens", "clearTokens") + const clearClientInfo = clearField("clientInfo", "clearClientInfo") const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier") const clearOAuthState = clearField("oauthState", "clearOAuthState") - const invalidateCredentials = Effect.fn("McpAuth.invalidateCredentials")(function* ( - mcpName: string, - type: "all" | "client" | "tokens", - expected: Pick, - ) { - yield* mutate((data) => { - const current = data[mcpName] - if (!current) return undefined - const entry = { ...current } - if ((type === "all" || type === "tokens") && credentialsEqual(current.tokens, expected.tokens)) - delete entry.tokens - if ((type === "all" || type === "client") && credentialsEqual(current.clientInfo, expected.clientInfo)) - delete entry.clientInfo - return { ...data, [mcpName]: entry } - }) - }) - const resetOAuthFlow = Effect.fn("McpAuth.resetOAuthFlow")(function* (mcpName: string) { yield* mutate((data) => { const current = data[mcpName] @@ -183,8 +164,9 @@ export const layer = Layer.effect( set, remove, updateTokens, + clearTokens, updateClientInfo, - invalidateCredentials, + clearClientInfo, updateCodeVerifier, clearCodeVerifier, updateOAuthState, @@ -199,8 +181,4 @@ export const defaultLayer = layer.pipe(Layer.provide(EffectFlock.defaultLayer), export const node = LayerNode.make({ service: Service, layer: layer, deps: [FSUtil.node, EffectFlock.node] }) -function credentialsEqual(left: Tokens | ClientInfo | undefined, right: Tokens | ClientInfo | undefined) { - return isDeepEqual(left, right) -} - export * as McpAuth from "./auth" diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index ce8b3a8ef73a..faec47297943 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -27,8 +27,6 @@ export interface McpOAuthCallbacks { export class McpOAuthProvider implements OAuthClientProvider { private pendingClientInfo?: OAuthClientInformationFull private pendingTokens?: OAuthTokens - private observedClientInfo?: McpAuth.ClientInfo - private observedTokens?: McpAuth.Tokens constructor( private mcpName: string, @@ -73,7 +71,6 @@ export class McpOAuthProvider implements OAuthClientProvider { // Use getForUrl to validate credentials are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (entry?.clientInfo) { - this.observedClientInfo = entry.clientInfo // Check if client secret has expired if (entry.clientInfo.clientSecretExpiresAt && entry.clientInfo.clientSecretExpiresAt < Date.now() / 1000) { return undefined @@ -93,13 +90,18 @@ export class McpOAuthProvider implements OAuthClientProvider { this.pendingClientInfo = info return } - this.observedClientInfo = { - clientId: info.client_id, - clientSecret: info.client_secret, - clientIdIssuedAt: info.client_id_issued_at, - clientSecretExpiresAt: info.client_secret_expires_at, - } - await Effect.runPromise(this.auth.updateClientInfo(this.mcpName, this.observedClientInfo, this.serverUrl)) + await Effect.runPromise( + this.auth.updateClientInfo( + this.mcpName, + { + clientId: info.client_id, + clientSecret: info.client_secret, + clientIdIssuedAt: info.client_id_issued_at, + clientSecretExpiresAt: info.client_secret_expires_at, + }, + this.serverUrl, + ), + ) } async tokens(): Promise { @@ -107,7 +109,6 @@ export class McpOAuthProvider implements OAuthClientProvider { // Use getForUrl to validate tokens are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (!entry?.tokens) return undefined - this.observedTokens = entry.tokens return { access_token: entry.tokens.accessToken, @@ -125,13 +126,18 @@ export class McpOAuthProvider implements OAuthClientProvider { this.pendingTokens = tokens return } - this.observedTokens = { - accessToken: tokens.access_token, - refreshToken: tokens.refresh_token, - expiresAt: tokens.expires_in ? Date.now() / 1000 + tokens.expires_in : undefined, - scope: tokens.scope, - } - await Effect.runPromise(this.auth.updateTokens(this.mcpName, this.observedTokens, this.serverUrl)) + await Effect.runPromise( + this.auth.updateTokens( + this.mcpName, + { + accessToken: tokens.access_token, + refreshToken: tokens.refresh_token, + expiresAt: tokens.expires_in ? Date.now() / 1000 + tokens.expires_in : undefined, + scope: tokens.scope, + }, + this.serverUrl, + ), + ) } async redirectToAuthorization(authorizationUrl: URL): Promise { @@ -177,12 +183,17 @@ export class McpOAuthProvider implements OAuthClientProvider { if (type === "all" || type === "tokens") this.pendingTokens = undefined return } - await Effect.runPromise( - this.auth.invalidateCredentials(this.mcpName, type, { - clientInfo: this.observedClientInfo, - tokens: this.observedTokens, - }), - ) + switch (type) { + case "all": + await Effect.runPromise(this.auth.remove(this.mcpName)) + break + case "client": + await Effect.runPromise(this.auth.clearClientInfo(this.mcpName)) + break + case "tokens": + await Effect.runPromise(this.auth.clearTokens(this.mcpName)) + break + } } async commit(): Promise { diff --git a/packages/opencode/test/mcp/auth.test.ts b/packages/opencode/test/mcp/auth.test.ts index 9fa77725bf0d..4593fad2600a 100644 --- a/packages/opencode/test/mcp/auth.test.ts +++ b/packages/opencode/test/mcp/auth.test.ts @@ -61,8 +61,9 @@ function gateCredentialInvalidation( const gate = Deferred.succeed(ready, undefined).pipe(Effect.ignore, Effect.andThen(Deferred.await(release))) return McpAuth.Service.of({ ...auth, - invalidateCredentials: (mcpName, type, expected) => - gate.pipe(Effect.andThen(auth.invalidateCredentials(mcpName, type, expected))), + get: (mcpName) => auth.get(mcpName).pipe(Effect.tap(() => gate)), + clearTokens: (mcpName) => gate.pipe(Effect.andThen(auth.clearTokens(mcpName))), + clearClientInfo: (mcpName) => gate.pipe(Effect.andThen(auth.clearClientInfo(mcpName))), }) } @@ -91,7 +92,7 @@ test("serializes concurrent auth file updates across service instances", async ( ) }) -test("stale token invalidation does not delete newer tokens", async () => { +test("concurrent token invalidation does not overwrite newer client registration", async () => { const name = `token-invalidation-${crypto.randomUUID()}` const url = "https://mcp.example.com/exact/path" @@ -110,22 +111,22 @@ test("stale token invalidation does not delete newer tokens", async () => { ) yield* first.updateTokens(name, { accessToken: "old-token" }, url) - yield* Effect.promise(() => provider.tokens()) const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("tokens")).pipe(Effect.forkChild) yield* Deferred.await(ready) - yield* second.updateTokens(name, { accessToken: "new-token" }, url) + yield* second.updateClientInfo(name, { clientId: "new-client" }, url) yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) yield* Fiber.join(invalidation) const entry = yield* first.get(name) - expect(entry?.tokens?.accessToken).toBe("new-token") + expect(entry?.tokens).toBeUndefined() + expect(entry?.clientInfo?.clientId).toBe("new-client") expect(entry?.serverUrl).toBe(url) yield* first.remove(name) }).pipe(Effect.provide(McpAuth.defaultLayer)), ) }) -test("stale client invalidation does not delete newer registration", async () => { +test("concurrent client invalidation does not overwrite newer tokens", async () => { const name = `client-invalidation-${crypto.randomUUID()}` const url = "https://mcp.example.com/exact/path" @@ -144,15 +145,15 @@ test("stale client invalidation does not delete newer registration", async () => ) yield* first.updateClientInfo(name, { clientId: "old-client" }, url) - yield* Effect.promise(() => provider.clientInformation()) const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("client")).pipe(Effect.forkChild) yield* Deferred.await(ready) - yield* second.updateClientInfo(name, { clientId: "new-client" }, url) + yield* second.updateTokens(name, { accessToken: "new-token" }, url) yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) yield* Fiber.join(invalidation) const entry = yield* first.get(name) - expect(entry?.clientInfo?.clientId).toBe("new-client") + expect(entry?.clientInfo).toBeUndefined() + expect(entry?.tokens?.accessToken).toBe("new-token") expect(entry?.serverUrl).toBe(url) yield* first.remove(name) }).pipe(Effect.provide(McpAuth.defaultLayer)), From 94d5ec39192dde00eb46b46f918e3c5042538678 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Fri, 26 Jun 2026 00:33:26 -0500 Subject: [PATCH 7/8] refactor(mcp): simplify reauthentication staging --- packages/opencode/src/mcp/auth.ts | 19 ----- packages/opencode/src/mcp/index.ts | 10 +-- packages/opencode/src/mcp/oauth-provider.ts | 24 +++--- packages/opencode/test/mcp/auth.test.ts | 85 +------------------ .../test/mcp/oauth-auto-connect.test.ts | 55 ++---------- 5 files changed, 22 insertions(+), 171 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index 137813eae5d9..f877824e0218 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -44,15 +44,12 @@ export interface Interface { readonly set: (mcpName: string, entry: Entry, serverUrl?: string) => Effect.Effect readonly remove: (mcpName: string) => Effect.Effect readonly updateTokens: (mcpName: string, tokens: Tokens, serverUrl?: string) => Effect.Effect - readonly clearTokens: (mcpName: string) => Effect.Effect readonly updateClientInfo: (mcpName: string, clientInfo: ClientInfo, serverUrl?: string) => Effect.Effect - readonly clearClientInfo: (mcpName: string) => Effect.Effect readonly updateCodeVerifier: (mcpName: string, codeVerifier: string) => Effect.Effect readonly clearCodeVerifier: (mcpName: string) => Effect.Effect readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect readonly getOAuthState: (mcpName: string) => Effect.Effect readonly clearOAuthState: (mcpName: string) => Effect.Effect - readonly resetOAuthFlow: (mcpName: string) => Effect.Effect } export class Service extends Context.Service()("@opencode/McpAuth") {} @@ -136,22 +133,9 @@ export const layer = Layer.effect( const updateClientInfo = updateField("clientInfo", "updateClientInfo") const updateCodeVerifier = updateField("codeVerifier", "updateCodeVerifier") const updateOAuthState = updateField("oauthState", "updateOAuthState") - const clearTokens = clearField("tokens", "clearTokens") - const clearClientInfo = clearField("clientInfo", "clearClientInfo") const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier") const clearOAuthState = clearField("oauthState", "clearOAuthState") - const resetOAuthFlow = Effect.fn("McpAuth.resetOAuthFlow")(function* (mcpName: string) { - yield* mutate((data) => { - const current = data[mcpName] - if (!current) return undefined - const entry = { ...current } - delete entry.codeVerifier - delete entry.oauthState - return { ...data, [mcpName]: entry } - }) - }) - const getOAuthState = Effect.fn("McpAuth.getOAuthState")(function* (mcpName: string) { const entry = yield* get(mcpName) return entry?.oauthState @@ -164,15 +148,12 @@ export const layer = Layer.effect( set, remove, updateTokens, - clearTokens, updateClientInfo, - clearClientInfo, updateCodeVerifier, clearCodeVerifier, updateOAuthState, getOAuthState, clearOAuthState, - resetOAuthFlow, }) }), ) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 7941e7a44f2b..e8b607189bfd 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -301,7 +301,7 @@ export const layer = Layer.effect( }) .pipe(Effect.ignore, Effect.as(undefined)) } else { - pendingOAuthTransports.set(key, { transport, provider: authProvider }) + pendingOAuthTransports.set(key, { transport }) lastStatus = { status: "needs_auth" as const } return events .publish(TuiEvent.ToastShow, { @@ -803,12 +803,6 @@ export const layer = Layer.effect( const url = remoteURL(mcpConfig.url) if (!url) throw new Error(`Invalid MCP URL for "${mcpName}"`) - const pending = pendingOAuthTransports.get(mcpName) - McpOAuthCallback.cancelPending(mcpName) - pendingOAuthTransports.delete(mcpName) - yield* Effect.tryPromise(() => pending?.transport.close() ?? Promise.resolve()).pipe(Effect.ignore) - yield* auth.resetOAuthFlow(mcpName) - // OAuth config is optional - if not provided, we'll use auto-discovery const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined @@ -833,7 +827,6 @@ export const layer = Layer.effect( clientSecret: oauthConfig?.clientSecret, scope: oauthConfig?.scope, redirectUri: effectiveRedirectUri, - deferPersistence: true, }, { onRedirect: async (url) => { @@ -841,6 +834,7 @@ export const layer = Layer.effect( }, }, auth, + { staged: true }, ) const transport = new StreamableHTTPClientTransport(url, { diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index faec47297943..cd994f6e94ea 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -17,7 +17,6 @@ export interface McpOAuthConfig { scope?: string callbackPort?: number redirectUri?: string - deferPersistence?: boolean } export interface McpOAuthCallbacks { @@ -34,6 +33,7 @@ export class McpOAuthProvider implements OAuthClientProvider { private config: McpOAuthConfig, private callbacks: McpOAuthCallbacks, private auth: McpAuth.Interface, + private options?: { staged: true }, ) {} get redirectUrl(): string { @@ -57,15 +57,13 @@ export class McpOAuthProvider implements OAuthClientProvider { } async clientInformation(): Promise { - // Check config first (pre-registered client) if (this.config.clientId) { return { client_id: this.config.clientId, client_secret: this.config.clientSecret, } } - - if (this.config.deferPersistence) return this.pendingClientInfo + if (this.options?.staged) return this.pendingClientInfo // Check stored client info (from dynamic registration) // Use getForUrl to validate credentials are for the current server URL @@ -86,7 +84,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveClientInformation(info: OAuthClientInformationFull): Promise { - if (this.config.deferPersistence) { + if (this.options?.staged) { this.pendingClientInfo = info return } @@ -105,7 +103,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async tokens(): Promise { - if (this.config.deferPersistence) return this.pendingTokens + if (this.options?.staged) return this.pendingTokens // Use getForUrl to validate tokens are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (!entry?.tokens) return undefined @@ -122,7 +120,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { - if (this.config.deferPersistence) { + if (this.options?.staged) { this.pendingTokens = tokens return } @@ -178,26 +176,30 @@ export class McpOAuthProvider implements OAuthClientProvider { } async invalidateCredentials(type: "all" | "client" | "tokens"): Promise { - if (this.config.deferPersistence) { + if (this.options?.staged) { if (type === "all" || type === "client") this.pendingClientInfo = undefined if (type === "all" || type === "tokens") this.pendingTokens = undefined return } + const entry = await Effect.runPromise(this.auth.get(this.mcpName)) + if (!entry) return switch (type) { case "all": await Effect.runPromise(this.auth.remove(this.mcpName)) break case "client": - await Effect.runPromise(this.auth.clearClientInfo(this.mcpName)) + delete entry.clientInfo + await Effect.runPromise(this.auth.set(this.mcpName, entry)) break case "tokens": - await Effect.runPromise(this.auth.clearTokens(this.mcpName)) + delete entry.tokens + await Effect.runPromise(this.auth.set(this.mcpName, entry)) break } } async commit(): Promise { - if (!this.config.deferPersistence || !this.pendingTokens) return + if (!this.pendingTokens) return await Effect.runPromise( this.auth.set( this.mcpName, diff --git a/packages/opencode/test/mcp/auth.test.ts b/packages/opencode/test/mcp/auth.test.ts index 4593fad2600a..0fdfe78b2af9 100644 --- a/packages/opencode/test/mcp/auth.test.ts +++ b/packages/opencode/test/mcp/auth.test.ts @@ -1,10 +1,9 @@ import { expect, test } from "bun:test" import { setTimeout as sleep } from "node:timers/promises" -import { Deferred, Effect, Fiber, Layer } from "effect" +import { Effect, Layer } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { McpAuth } from "../../src/mcp/auth" -import { McpOAuthProvider } from "../../src/mcp/oauth-provider" function authFile() { let raw = "" @@ -53,20 +52,6 @@ function authService(layer: Layer.Layer) { ) } -function gateCredentialInvalidation( - auth: McpAuth.Interface, - ready: Deferred.Deferred, - release: Deferred.Deferred, -) { - const gate = Deferred.succeed(ready, undefined).pipe(Effect.ignore, Effect.andThen(Deferred.await(release))) - return McpAuth.Service.of({ - ...auth, - get: (mcpName) => auth.get(mcpName).pipe(Effect.tap(() => gate)), - clearTokens: (mcpName) => gate.pipe(Effect.andThen(auth.clearTokens(mcpName))), - clearClientInfo: (mcpName) => gate.pipe(Effect.andThen(auth.clearClientInfo(mcpName))), - }) -} - test("serializes concurrent auth file updates across service instances", async () => { const file = authFile() @@ -91,71 +76,3 @@ test("serializes concurrent auth file updates across service instances", async ( }), ) }) - -test("concurrent token invalidation does not overwrite newer client registration", async () => { - const name = `token-invalidation-${crypto.randomUUID()}` - const url = "https://mcp.example.com/exact/path" - - await Effect.runPromise( - Effect.gen(function* () { - const first = yield* McpAuth.Service - const second = yield* authService(FSUtil.defaultLayer) - const ready = yield* Deferred.make() - const release = yield* Deferred.make() - const provider = new McpOAuthProvider( - name, - url, - {}, - { onRedirect: async () => {} }, - gateCredentialInvalidation(first, ready, release), - ) - - yield* first.updateTokens(name, { accessToken: "old-token" }, url) - const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("tokens")).pipe(Effect.forkChild) - yield* Deferred.await(ready) - yield* second.updateClientInfo(name, { clientId: "new-client" }, url) - yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) - yield* Fiber.join(invalidation) - - const entry = yield* first.get(name) - expect(entry?.tokens).toBeUndefined() - expect(entry?.clientInfo?.clientId).toBe("new-client") - expect(entry?.serverUrl).toBe(url) - yield* first.remove(name) - }).pipe(Effect.provide(McpAuth.defaultLayer)), - ) -}) - -test("concurrent client invalidation does not overwrite newer tokens", async () => { - const name = `client-invalidation-${crypto.randomUUID()}` - const url = "https://mcp.example.com/exact/path" - - await Effect.runPromise( - Effect.gen(function* () { - const first = yield* McpAuth.Service - const second = yield* authService(FSUtil.defaultLayer) - const ready = yield* Deferred.make() - const release = yield* Deferred.make() - const provider = new McpOAuthProvider( - name, - url, - {}, - { onRedirect: async () => {} }, - gateCredentialInvalidation(first, ready, release), - ) - - yield* first.updateClientInfo(name, { clientId: "old-client" }, url) - const invalidation = yield* Effect.promise(() => provider.invalidateCredentials("client")).pipe(Effect.forkChild) - yield* Deferred.await(ready) - yield* second.updateTokens(name, { accessToken: "new-token" }, url) - yield* Deferred.succeed(release, undefined).pipe(Effect.ignore) - yield* Fiber.join(invalidation) - - const entry = yield* first.get(name) - expect(entry?.clientInfo).toBeUndefined() - expect(entry?.tokens?.accessToken).toBe("new-token") - expect(entry?.serverUrl).toBe(url) - yield* first.remove(name) - }).pipe(Effect.provide(McpAuth.defaultLayer)), - ) -}) diff --git a/packages/opencode/test/mcp/oauth-auto-connect.test.ts b/packages/opencode/test/mcp/oauth-auto-connect.test.ts index 7e8f26af92f6..43b10929e4bb 100644 --- a/packages/opencode/test/mcp/oauth-auto-connect.test.ts +++ b/packages/opencode/test/mcp/oauth-auto-connect.test.ts @@ -23,7 +23,6 @@ let simulateAuthFlow = true let connectSucceedsImmediately = false let serverCapabilities: { tools?: object; resources?: object } = { tools: {} } let listToolsCalls = 0 -let transportCloseCount = 0 let finishAuthFails = false let finishAuthStoresCredentials = false @@ -36,6 +35,7 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ redirectToAuthorization?: (url: URL) => Promise saveCodeVerifier?: (v: string) => Promise tokens?: () => Promise<{ access_token: string } | undefined> + clientInformation?: () => Promise<{ client_id: string } | undefined> saveClientInformation?: (info: { client_id: string; client_secret?: string }) => Promise saveTokens?: (tokens: { access_token: string; token_type: string }) => Promise } @@ -56,6 +56,7 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ // provider.redirectToAuthorization(), then throws UnauthorizedError. if (simulateAuthFlow && this.authProvider) { if (await this.authProvider.tokens?.()) throw new MockUnauthorizedError() + if (await this.authProvider.clientInformation?.()) throw new MockUnauthorizedError() // The SDK calls provider.state() to get the OAuth state parameter if (this.authProvider.state) { await this.authProvider.state() @@ -79,9 +80,7 @@ void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ await this.authProvider?.saveTokens?.({ access_token: "replacement-token", token_type: "Bearer" }) } } - async close() { - transportCloseCount++ - } + async close() {} }, })) @@ -141,7 +140,6 @@ beforeEach(() => { connectSucceedsImmediately = false serverCapabilities = { tools: {} } listToolsCalls = 0 - transportCloseCount = 0 finishAuthFails = false finishAuthStoresCredentials = false }) @@ -247,48 +245,6 @@ mcpTest.instance("state() returns existing state when one is saved", () => }), ) -mcpTest.instance( - "reauthentication preserves existing credentials until replacement succeeds", - () => - Effect.gen(function* () { - yield* Effect.addFinalizer(() => Effect.promise(() => McpOAuthCallback.stop()).pipe(Effect.ignore)) - const mcp = yield* MCP.Service - const auth = yield* McpAuth.Service - const name = "test-reauth" - const url = "https://example.com/mcp" - const clientInfo = { clientId: "dynamic-client", clientSecret: "dynamic-secret" } - - yield* auth.updateClientInfo(name, clientInfo, url) - yield* auth.updateTokens(name, { accessToken: "stale-token" }, url) - yield* auth.updateCodeVerifier(name, "stale-verifier") - yield* auth.updateOAuthState(name, "stale-state") - - const first = yield* mcp.startAuth(name) - const afterFirst = yield* auth.get(name) - - expect(first.authorizationUrl).toContain("https://auth.example.com/authorize") - expect(first.oauthState).not.toBe("stale-state") - expect(afterFirst?.tokens?.accessToken).toBe("stale-token") - expect(afterFirst?.clientInfo).toEqual(clientInfo) - expect(afterFirst?.serverUrl).toBe(url) - expect(afterFirst?.codeVerifier).toBe("test-verifier") - expect(afterFirst?.oauthState).toBe(first.oauthState) - - const cancelled = McpOAuthCallback.waitForCallback(first.oauthState, name).catch((error) => error) - const closedBeforeRestart = transportCloseCount - yield* mcp.startAuth(name) - expect(transportCloseCount).toBe(closedBeforeRestart + 1) - const cancellation = yield* Effect.promise(() => cancelled) - expect(cancellation).toBeInstanceOf(Error) - if (!(cancellation instanceof Error)) throw new Error("Expected abandoned authorization to be cancelled") - expect(cancellation.message).toBe("Authorization cancelled") - const afterCancellation = yield* auth.get(name) - expect(afterCancellation?.tokens?.accessToken).toBe("stale-token") - expect(afterCancellation?.clientInfo).toEqual(clientInfo) - }), - { config: config("test-reauth") }, -) - mcpTest.instance( "failed reauthentication preserves existing credentials", () => @@ -302,7 +258,7 @@ mcpTest.instance( yield* auth.updateClientInfo(name, clientInfo, url) yield* auth.updateTokens(name, { accessToken: "working-token" }, url) - yield* mcp.startAuth(name) + expect((yield* mcp.startAuth(name)).authorizationUrl).toContain("https://auth.example.com/authorize") finishAuthFails = true expect(yield* mcp.finishAuth(name, "invalid-code")).toEqual({ @@ -328,7 +284,8 @@ mcpTest.instance( yield* auth.updateClientInfo(name, { clientId: "old-client" }, url) yield* auth.updateTokens(name, { accessToken: "old-token" }, url) - yield* mcp.startAuth(name) + expect((yield* mcp.startAuth(name)).authorizationUrl).toContain("https://auth.example.com/authorize") + expect((yield* auth.get(name))?.tokens?.accessToken).toBe("old-token") finishAuthStoresCredentials = true connectSucceedsImmediately = true From b36e8d439f182e82fb853761d9d53877a64c8851 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Fri, 26 Jun 2026 00:45:32 -0500 Subject: [PATCH 8/8] refactor(mcp): separate pending oauth credentials --- packages/opencode/src/mcp/index.ts | 7 ++- packages/opencode/src/mcp/oauth-provider.ts | 57 ++++++++++++--------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index e8b607189bfd..7f6141ca3da2 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -22,7 +22,7 @@ import { NamedError } from "@opencode-ai/core/util/error" import { InstallationVersion } from "@opencode-ai/core/installation/version" import { withTimeout } from "@/util/timeout" import { FSUtil } from "@opencode-ai/core/fs-util" -import { McpOAuthProvider, OAUTH_CALLBACK_PATH } from "./oauth-provider" +import { McpOAuthPendingProvider, McpOAuthProvider, OAUTH_CALLBACK_PATH } from "./oauth-provider" import { McpOAuthCallback } from "./oauth-callback" import { McpAuth } from "./auth" import { EventV2Bridge } from "@/event-v2-bridge" @@ -109,7 +109,7 @@ export type Status = Schema.Schema.Type // Store transports for OAuth servers to allow finishing auth type TransportWithAuth = StreamableHTTPClientTransport | SSEClientTransport -const pendingOAuthTransports = new Map() +const pendingOAuthTransports = new Map() // Prompt cache types type PromptInfo = Awaited>["prompts"][number] @@ -819,7 +819,7 @@ export const layer = Layer.effect( .join("") yield* auth.updateOAuthState(mcpName, oauthState) let capturedUrl: URL | undefined - const authProvider = new McpOAuthProvider( + const authProvider = new McpOAuthPendingProvider( mcpName, mcpConfig.url, { @@ -834,7 +834,6 @@ export const layer = Layer.effect( }, }, auth, - { staged: true }, ) const transport = new StreamableHTTPClientTransport(url, { diff --git a/packages/opencode/src/mcp/oauth-provider.ts b/packages/opencode/src/mcp/oauth-provider.ts index cd994f6e94ea..596bfe1d551f 100644 --- a/packages/opencode/src/mcp/oauth-provider.ts +++ b/packages/opencode/src/mcp/oauth-provider.ts @@ -24,16 +24,12 @@ export interface McpOAuthCallbacks { } export class McpOAuthProvider implements OAuthClientProvider { - private pendingClientInfo?: OAuthClientInformationFull - private pendingTokens?: OAuthTokens - constructor( - private mcpName: string, - private serverUrl: string, - private config: McpOAuthConfig, + protected mcpName: string, + protected serverUrl: string, + protected config: McpOAuthConfig, private callbacks: McpOAuthCallbacks, - private auth: McpAuth.Interface, - private options?: { staged: true }, + protected auth: McpAuth.Interface, ) {} get redirectUrl(): string { @@ -63,7 +59,6 @@ export class McpOAuthProvider implements OAuthClientProvider { client_secret: this.config.clientSecret, } } - if (this.options?.staged) return this.pendingClientInfo // Check stored client info (from dynamic registration) // Use getForUrl to validate credentials are for the current server URL @@ -84,10 +79,6 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveClientInformation(info: OAuthClientInformationFull): Promise { - if (this.options?.staged) { - this.pendingClientInfo = info - return - } await Effect.runPromise( this.auth.updateClientInfo( this.mcpName, @@ -103,7 +94,6 @@ export class McpOAuthProvider implements OAuthClientProvider { } async tokens(): Promise { - if (this.options?.staged) return this.pendingTokens // Use getForUrl to validate tokens are for the current server URL const entry = await Effect.runPromise(this.auth.getForUrl(this.mcpName, this.serverUrl)) if (!entry?.tokens) return undefined @@ -120,10 +110,6 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { - if (this.options?.staged) { - this.pendingTokens = tokens - return - } await Effect.runPromise( this.auth.updateTokens( this.mcpName, @@ -176,11 +162,6 @@ export class McpOAuthProvider implements OAuthClientProvider { } async invalidateCredentials(type: "all" | "client" | "tokens"): Promise { - if (this.options?.staged) { - if (type === "all" || type === "client") this.pendingClientInfo = undefined - if (type === "all" || type === "tokens") this.pendingTokens = undefined - return - } const entry = await Effect.runPromise(this.auth.get(this.mcpName)) if (!entry) return switch (type) { @@ -197,6 +178,36 @@ export class McpOAuthProvider implements OAuthClientProvider { break } } +} + +export class McpOAuthPendingProvider extends McpOAuthProvider { + private pendingClientInfo?: OAuthClientInformationFull + private pendingTokens?: OAuthTokens + + override async clientInformation(): Promise { + if (!this.config.clientId) return this.pendingClientInfo + return { + client_id: this.config.clientId, + client_secret: this.config.clientSecret, + } + } + + override async saveClientInformation(info: OAuthClientInformationFull): Promise { + this.pendingClientInfo = info + } + + override async tokens(): Promise { + return this.pendingTokens + } + + override async saveTokens(tokens: OAuthTokens): Promise { + this.pendingTokens = tokens + } + + override async invalidateCredentials(type: "all" | "client" | "tokens"): Promise { + if (type === "all" || type === "client") this.pendingClientInfo = undefined + if (type === "all" || type === "tokens") this.pendingTokens = undefined + } async commit(): Promise { if (!this.pendingTokens) return