From ed486f0b497ba6bb7edfe11a0a28f9fb95c4a4e5 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Wed, 24 Jun 2026 16:29:15 -0500 Subject: [PATCH 1/2] fix(opencode): secure manual MCP OAuth callback --- packages/opencode/src/mcp/auth.ts | 14 + packages/opencode/src/mcp/index.ts | 111 +++++--- .../routes/instance/httpapi/groups/mcp.ts | 1 + .../routes/instance/httpapi/handlers/mcp.ts | 12 +- .../test/server/httpapi-mcp-oauth.test.ts | 241 ++++++++++++++---- packages/sdk/js/src/v2/gen/sdk.gen.ts | 2 + packages/sdk/js/src/v2/gen/types.gen.ts | 1 + 7 files changed, 283 insertions(+), 99 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index be03760c52b4..cd829dd38d47 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -49,6 +49,7 @@ export interface Interface { readonly clearCodeVerifier: (mcpName: string) => Effect.Effect readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect readonly getOAuthState: (mcpName: string) => Effect.Effect + readonly consumeOAuthState: (mcpName: string, oauthState: string) => Effect.Effect readonly clearOAuthState: (mcpName: string) => Effect.Effect readonly isTokenExpired: (mcpName: string) => Effect.Effect } @@ -142,6 +143,18 @@ export const layer = Layer.effect( return entry?.oauthState }) + const consumeOAuthState = Effect.fn("McpAuth.consumeOAuthState")(function* (mcpName: string, oauthState: string) { + return yield* Effect.gen(function* () { + const data = yield* read() + const entry = data[mcpName] + if (!entry?.oauthState) return false + const matches = entry.oauthState === oauthState + delete entry.oauthState + yield* fs.writeJson(filepath, { ...data, [mcpName]: entry }, 0o600).pipe(Effect.orDie) + return matches + }).pipe(flock.withLock(lockKey), Effect.orDie) + }) + const isTokenExpired = Effect.fn("McpAuth.isTokenExpired")(function* (mcpName: string) { const entry = yield* get(mcpName) if (!entry?.tokens) return null @@ -161,6 +174,7 @@ export const layer = Layer.effect( clearCodeVerifier, updateOAuthState, getOAuthState, + consumeOAuthState, clearOAuthState, isTokenExpired, }) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index db673244b3d7..90b136a8e871 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -71,6 +71,10 @@ export class NotFoundError extends Schema.TaggedErrorClass()("MCP name: Schema.String, }) {} +export class OAuthError extends Schema.TaggedErrorClass()("MCP.OAuthError", { + message: Schema.String, +}) {} + type MCPClient = Client function createClient(directory: string) { @@ -180,7 +184,11 @@ export interface Interface { mcpName: string, ) => Effect.Effect<{ authorizationUrl: string; oauthState: string }, NotFoundError> readonly authenticate: (mcpName: string) => Effect.Effect - readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect + readonly finishAuth: ( + mcpName: string, + authorizationCode: string, + oauthState: string, + ) => Effect.Effect readonly removeAuth: (mcpName: string) => Effect.Effect readonly supportsOAuth: (mcpName: string) => Effect.Effect readonly hasStoredTokens: (mcpName: string) => Effect.Effect @@ -793,6 +801,16 @@ export const layer = Layer.effect( return mcpConfig }) + const cleanupAuth = Effect.fnUntraced(function* (mcpName: string) { + const transport = pendingOAuthTransports.get(mcpName) + pendingOAuthTransports.delete(mcpName) + yield* Effect.all([ + auth.clearOAuthState(mcpName), + auth.clearCodeVerifier(mcpName), + Effect.tryPromise(() => transport?.close() ?? Promise.resolve()).pipe(Effect.ignore), + ]) + }) + const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { const mcpConfig = yield* requireMcpConfig(mcpName) if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) @@ -808,8 +826,7 @@ export const layer = Layer.effect( oauthConfig?.redirectUri ?? (oauthConfig?.callbackPort ? `http://127.0.0.1:${oauthConfig.callbackPort}${OAUTH_CALLBACK_PATH}` : undefined) - // Start the callback server with custom redirectUri if configured - yield* Effect.promise(() => McpOAuthCallback.ensureRunning(effectiveRedirectUri)) + yield* cleanupAuth(mcpName) const oauthState = Array.from(crypto.getRandomValues(new Uint8Array(32))) .map((b) => b.toString(16).padStart(2, "0")) @@ -853,7 +870,7 @@ export const layer = Layer.effect( pendingOAuthTransports.set(mcpName, transport) return Effect.succeed({ authorizationUrl: capturedUrl.toString(), oauthState } satisfies AuthResult) } - return Effect.die(error) + return cleanupAuth(mcpName).pipe(Effect.andThen(Effect.die(error))) }), ) }) @@ -881,44 +898,66 @@ export const layer = Layer.effect( return yield* storeClient(s, mcpName, client, listed, client.getInstructions()?.trim(), mcpConfig.timeout) } - const callbackPromise = McpOAuthCallback.waitForCallback(result.oauthState, mcpName) - - yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( - Effect.flatMap((subprocess) => - Effect.callback((resume) => { - const timer = setTimeout(() => resume(Effect.void), 500) - subprocess.on("error", (err) => { - clearTimeout(timer) - resume(Effect.fail(err)) - }) - subprocess.on("exit", (code) => { - if (code !== null && code !== 0) { + return yield* Effect.gen(function* () { + const mcpConfig = yield* requireMcpConfig(mcpName) + if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) + const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined + yield* Effect.promise(() => + McpOAuthCallback.ensureRunning( + oauthConfig?.redirectUri ?? + (oauthConfig?.callbackPort + ? `http://127.0.0.1:${oauthConfig.callbackPort}${OAUTH_CALLBACK_PATH}` + : undefined), + ), + ) + + const callbackPromise = McpOAuthCallback.waitForCallback(result.oauthState, mcpName) + + yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( + Effect.flatMap((subprocess) => + Effect.callback((resume) => { + const timer = setTimeout(() => resume(Effect.void), 500) + subprocess.on("error", (err) => { clearTimeout(timer) - resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) - } - }) + resume(Effect.fail(err)) + }) + subprocess.on("exit", (code) => { + if (code !== null && code !== 0) { + clearTimeout(timer) + resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) + } + }) + }), + ), + Effect.catch(() => { + return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) }), + ) + + const code = yield* Effect.promise(() => callbackPromise) + return yield* finishAuth(mcpName, code, result.oauthState).pipe( + Effect.catchTag("MCP.OAuthError", (error) => Effect.die(error)), + ) + }).pipe( + Effect.ensuring( + Effect.sync(() => McpOAuthCallback.cancelPending(mcpName)).pipe(Effect.andThen(cleanupAuth(mcpName))), ), - Effect.catch(() => { - return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) - }), ) - - const code = yield* Effect.promise(() => callbackPromise) - - const storedState = yield* auth.getOAuthState(mcpName) - if (storedState !== result.oauthState) { - yield* auth.clearOAuthState(mcpName) - throw new Error("OAuth state mismatch - potential CSRF attack") - } - yield* auth.clearOAuthState(mcpName) - return yield* finishAuth(mcpName, code) }) - const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { + const finishAuth = Effect.fn("MCP.finishAuth")(function* ( + mcpName: string, + authorizationCode: string, + oauthState: string, + ) { yield* requireMcpConfig(mcpName) const transport = pendingOAuthTransports.get(mcpName) - if (!transport) throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) + if (!transport) return yield* new OAuthError({ message: `No pending OAuth flow for MCP server: ${mcpName}` }) + + if (!(yield* auth.consumeOAuthState(mcpName, oauthState))) { + yield* cleanupAuth(mcpName) + return yield* new OAuthError({ message: "Invalid or expired OAuth state - potential CSRF attack" }) + } const result = yield* Effect.tryPromise({ try: () => transport.finishAuth(authorizationCode).then(() => true as const), @@ -927,13 +966,11 @@ export const layer = Layer.effect( }, }).pipe(Effect.option) + yield* cleanupAuth(mcpName) if (Option.isNone(result)) { return { status: "failed", error: "OAuth completion failed" } satisfies Status } - yield* auth.clearCodeVerifier(mcpName) - pendingOAuthTransports.delete(mcpName) - const mcpConfig = yield* requireMcpConfig(mcpName) return yield* createAndStore(mcpName, mcpConfig) diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts index a6fb064d73e4..e4bae1a0daa8 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts @@ -20,6 +20,7 @@ export const AuthStartResponse = Schema.Struct({ }) export const AuthCallbackPayload = Schema.Struct({ code: Schema.String, + state: Schema.String, }) export const AuthRemoveResponse = Schema.Struct({ success: Schema.Literal(true), diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts index cdf0cc1e70eb..2b0e686f5a7e 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts @@ -37,15 +37,15 @@ export const mcpHandlers = HttpApiBuilder.group(InstanceHttpApi, "mcp", (handler params: { name: string } payload: typeof AuthCallbackPayload.Type }) { - return yield* mcp - .finishAuth(ctx.params.name, ctx.payload.code) - .pipe( - Effect.catchTag("MCP.NotFoundError", (error) => + return yield* mcp.finishAuth(ctx.params.name, ctx.payload.code, ctx.payload.state).pipe( + Effect.catchTags({ + "MCP.NotFoundError": (error) => Effect.fail( new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` }), ), - ), - ) + "MCP.OAuthError": () => Effect.fail(new HttpApiError.BadRequest({})), + }), + ) }) const authAuthenticate = Effect.fn("McpHttpApi.authAuthenticate")(function* (ctx: { params: { name: string } }) { diff --git a/packages/opencode/test/server/httpapi-mcp-oauth.test.ts b/packages/opencode/test/server/httpapi-mcp-oauth.test.ts index d3ca4ae6835b..d0f8690fdda8 100644 --- a/packages/opencode/test/server/httpapi-mcp-oauth.test.ts +++ b/packages/opencode/test/server/httpapi-mcp-oauth.test.ts @@ -1,73 +1,202 @@ -import { NodeHttpServer } from "@effect/platform-node" -import { Session } from "@/session/session" +import { NodeHttpServer, NodeServices } from "@effect/platform-node" +import Http from "node:http" +import path from "node:path" import { describe, expect } from "bun:test" -import { Effect, Layer } from "effect" -import { HttpClient, HttpClientRequest, HttpRouter } from "effect/unstable/http" -import { HttpApi, HttpApiBuilder } from "effect/unstable/httpapi" -import { McpApi, McpPaths } from "../../src/server/routes/instance/httpapi/groups/mcp" -import { Authorization } from "../../src/server/routes/instance/httpapi/middleware/authorization" -import { InstanceContextMiddleware } from "../../src/server/routes/instance/httpapi/middleware/instance-context" +import { Config, Context, Effect, Layer } from "effect" import { - WorkspaceRouteContext, - WorkspaceRoutingMiddleware, -} from "../../src/server/routes/instance/httpapi/middleware/workspace-routing" + HttpClient, + HttpClientRequest, + HttpRouter, + HttpServer, + HttpServerRequest, + HttpServerResponse, +} from "effect/unstable/http" +import { McpOAuthCallback } from "../../src/mcp/oauth-callback" +import { McpPaths } from "../../src/server/routes/instance/httpapi/groups/mcp" +import { HttpApiApp } from "../../src/server/routes/instance/httpapi/server" +import { tmpdirScoped } from "../fixture/fixture" import { testEffect } from "../lib/effect" -const TestHttpApi = HttpApi.make("opencode-instance").addHttpApi(McpApi) -const fakeSession = Layer.mock(Session.Service)({}) -const testMcpHandlers = HttpApiBuilder.group(TestHttpApi, "mcp", (handlers) => - Effect.succeed( - handlers - .handle("status", () => Effect.die("unexpected MCP status")) - .handle("add", () => Effect.die("unexpected MCP add")) - .handle("authStart", () => - Effect.succeed({ authorizationUrl: "https://auth.example/start", oauthState: "state-123" }), - ) - .handle("authCallback", () => Effect.die("unexpected MCP authCallback")) - .handle("authAuthenticate", () => Effect.die("unexpected MCP authAuthenticate")) - .handle("authRemove", () => Effect.die("unexpected MCP authRemove")) - .handle("connect", () => Effect.die("unexpected MCP connect")) - .handle("disconnect", () => Effect.die("unexpected MCP disconnect")), - ), +const servedRoutes: Layer.Layer = HttpRouter.serve( + HttpApiApp.routes, + { + disableListenLog: true, + disableLogger: true, + }, ) -const passthroughAuthorization = Layer.succeed( - Authorization, - Authorization.of((effect) => effect), +const it = testEffect( + servedRoutes.pipe(Layer.provideMerge(NodeHttpServer.layerTest), Layer.provideMerge(NodeServices.layer)), ) -const passthroughInstanceContext = Layer.succeed( - InstanceContextMiddleware, - InstanceContextMiddleware.of((effect) => effect), -) +const callbackPath = McpPaths.authCallback.replace(":name", "secure-oauth") +const authPath = McpPaths.auth.replace(":name", "secure-oauth") -const testWorkspaceRouting = Layer.succeed( - WorkspaceRoutingMiddleware, - WorkspaceRoutingMiddleware.of((effect) => - effect.pipe(Effect.provideService(WorkspaceRouteContext, WorkspaceRouteContext.of({ directory: process.cwd() }))), - ), -) +function listenOAuthServer(tokenCalls: { value: number }) { + return Effect.gen(function* () { + const context = yield* Layer.build(NodeHttpServer.layer(Http.createServer, { host: "127.0.0.1", port: 0 })) + const server = Context.get(context, HttpServer.HttpServer) + const origin = HttpServer.formatAddress(server.address) + yield* server.serve( + HttpServerRequest.HttpServerRequest.use((request) => { + const url = new URL(request.url, origin) + if (url.pathname === "/.well-known/oauth-protected-resource/mcp") + return HttpServerResponse.json({ + resource: `${origin}/mcp`, + authorization_servers: [origin], + }) + if (url.pathname === "/.well-known/oauth-authorization-server") + return HttpServerResponse.json({ + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + token_endpoint: `${origin}/token`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + code_challenge_methods_supported: ["S256"], + }) + if (url.pathname === "/token") + return Effect.gen(function* () { + yield* request.text + tokenCalls.value++ + return yield* HttpServerResponse.json({ access_token: "access-token", token_type: "Bearer" }) + }) + if (url.pathname !== "/mcp") return Effect.succeed(HttpServerResponse.empty({ status: 404 })) + if (request.headers.authorization !== "Bearer access-token") + return Effect.succeed( + HttpServerResponse.empty({ + status: 401, + headers: { + "www-authenticate": `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource/mcp"`, + }, + }), + ) + return Effect.gen(function* () { + const body = yield* request.json + if ( + typeof body === "object" && + body !== null && + "method" in body && + body.method === "notifications/initialized" + ) + return HttpServerResponse.empty({ status: 202 }) + if (typeof body === "object" && body !== null && "method" in body && body.method === "tools/list") + return yield* HttpServerResponse.json({ + jsonrpc: "2.0", + id: "id" in body ? body.id : null, + result: { tools: [] }, + }) + return yield* HttpServerResponse.json({ + jsonrpc: "2.0", + id: typeof body === "object" && body !== null && "id" in body ? body.id : null, + result: { + protocolVersion: "2025-03-26", + capabilities: { tools: {} }, + serverInfo: { name: "oauth-test", version: "1.0.0" }, + }, + }) + }) + }), + ) + return origin + }) +} -const it = testEffect( - HttpRouter.serve( - HttpApiBuilder.layer(TestHttpApi).pipe( - Layer.provide(testMcpHandlers), - Layer.provide([passthroughAuthorization, passthroughInstanceContext, testWorkspaceRouting, fakeSession]), - ), - { disableListenLog: true, disableLogger: true }, - ).pipe(Layer.provideMerge(NodeHttpServer.layerTest)), -) +function availablePort() { + return Effect.promise( + () => + new Promise((resolve, reject) => { + const server = Http.createServer() + server.on("error", reject) + server.listen(0, "127.0.0.1", () => { + const address = server.address() + if (!address || typeof address === "string") return reject(new Error("Failed to allocate callback port")) + server.close((error) => (error ? reject(error) : resolve(address.port))) + }) + }), + ) +} + +function assertPortAvailable(port: number) { + return Effect.promise( + () => + new Promise((resolve, reject) => { + const server = Http.createServer() + server.on("error", reject) + server.listen(port, "127.0.0.1", () => server.close((error) => (error ? reject(error) : resolve()))) + }), + ) +} + +function setup() { + return Effect.gen(function* () { + const directory = yield* tmpdirScoped({ git: true }) + const tokenCalls = { value: 0 } + const upstream = yield* listenOAuthServer(tokenCalls) + const callbackPort = yield* availablePort() + yield* Effect.promise(() => + Bun.write( + path.join(directory, "opencode.json"), + JSON.stringify({ + formatter: false, + lsp: false, + mcp: { + "secure-oauth": { + type: "remote", + url: `${upstream}/mcp`, + oauth: { clientId: "test-client", callbackPort }, + }, + }, + }), + ), + ) + return { directory, tokenCalls, callbackPort } + }) +} + +function request(directory: string, route: string, payload?: object) { + const base = HttpClientRequest.post(route).pipe(HttpClientRequest.setHeader("x-opencode-directory", directory)) + if (!payload) return HttpClient.execute(base) + return base.pipe(HttpClientRequest.bodyJson(payload), Effect.flatMap(HttpClient.execute)) +} describe("mcp HttpApi OAuth", () => { - it.live("preserves oauth state when starting OAuth", () => + it.live("requires, validates, consumes, and rejects replayed callback state", () => Effect.gen(function* () { - const response = yield* HttpClientRequest.post(McpPaths.auth.replace(":name", "demo")).pipe(HttpClient.execute) + const test = yield* setup() + const started = yield* request(test.directory, authPath) + expect(started.status).toBe(200) + const first = (yield* started.json) as { oauthState: string } + + const missing = yield* request(test.directory, callbackPath, { code: "missing-state" }) + expect(missing.status).toBe(400) + expect(test.tokenCalls.value).toBe(0) + + const wrong = yield* request(test.directory, callbackPath, { code: "wrong-state", state: "wrong" }) + expect(wrong.status).toBe(400) + expect(test.tokenCalls.value).toBe(0) + + const restarted = yield* request(test.directory, authPath) + expect(restarted.status).toBe(200) + const second = (yield* restarted.json) as { oauthState: string } + expect(second.oauthState).not.toBe(first.oauthState) + + const correct = yield* request(test.directory, callbackPath, { code: "valid-code", state: second.oauthState }) + expect(correct.status).toBe(200) + expect(test.tokenCalls.value).toBe(1) - expect(response.status).toBe(200) - expect(yield* response.json).toEqual({ - authorizationUrl: "https://auth.example/start", - oauthState: "state-123", - }) + const replayed = yield* request(test.directory, callbackPath, { code: "replayed-code", state: second.oauthState }) + expect(replayed.status).toBe(400) + expect(test.tokenCalls.value).toBe(1) + }), + ) + + it.live("does not bind the browser callback listener during manual start", () => + Effect.gen(function* () { + const test = yield* setup() + const started = yield* request(test.directory, authPath) + expect(started.status).toBe(200) + expect(McpOAuthCallback.isRunning()).toBe(false) + yield* assertPortAvailable(test.callbackPort) }), ) }) diff --git a/packages/sdk/js/src/v2/gen/sdk.gen.ts b/packages/sdk/js/src/v2/gen/sdk.gen.ts index d501e28132cd..0a09ff3d4de3 100644 --- a/packages/sdk/js/src/v2/gen/sdk.gen.ts +++ b/packages/sdk/js/src/v2/gen/sdk.gen.ts @@ -2304,6 +2304,7 @@ export class Auth2 extends HeyApiClient { directory?: string workspace?: string code?: string + state?: string }, options?: Options, ) { @@ -2316,6 +2317,7 @@ export class Auth2 extends HeyApiClient { { in: "query", key: "directory" }, { in: "query", key: "workspace" }, { in: "body", key: "code" }, + { in: "body", key: "state" }, ], }, ], diff --git a/packages/sdk/js/src/v2/gen/types.gen.ts b/packages/sdk/js/src/v2/gen/types.gen.ts index 3116c295f125..5465e085c1eb 100644 --- a/packages/sdk/js/src/v2/gen/types.gen.ts +++ b/packages/sdk/js/src/v2/gen/types.gen.ts @@ -8407,6 +8407,7 @@ export type McpAuthStartResponse = McpAuthStartResponses[keyof McpAuthStartRespo export type McpAuthCallbackData = { body?: { code: string + state: string } path: { name: string From bb388efae65e9fff470d396489ccd35e7cbe359a Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Wed, 24 Jun 2026 16:59:34 -0500 Subject: [PATCH 2/2] fix(opencode): preserve valid MCP OAuth state --- packages/opencode/src/mcp/auth.ts | 5 ++-- packages/opencode/src/mcp/index.ts | 10 +++++--- .../routes/instance/httpapi/groups/mcp.ts | 2 +- .../test/server/httpapi-mcp-oauth.test.ts | 25 ++++++++----------- .../opencode/test/server/httpapi-mcp.test.ts | 6 ++++- packages/sdk/js/src/v2/gen/sdk.gen.ts | 2 +- 6 files changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index cd829dd38d47..d48ebc0d1ec1 100644 --- a/packages/opencode/src/mcp/auth.ts +++ b/packages/opencode/src/mcp/auth.ts @@ -147,11 +147,10 @@ export const layer = Layer.effect( return yield* Effect.gen(function* () { const data = yield* read() const entry = data[mcpName] - if (!entry?.oauthState) return false - const matches = entry.oauthState === oauthState + if (entry?.oauthState !== oauthState) return false delete entry.oauthState yield* fs.writeJson(filepath, { ...data, [mcpName]: entry }, 0o600).pipe(Effect.orDie) - return matches + return true }).pipe(flock.withLock(lockKey), Effect.orDie) }) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 90b136a8e871..f0a1e616a0d8 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -951,14 +951,16 @@ export const layer = Layer.effect( oauthState: string, ) { yield* requireMcpConfig(mcpName) - const transport = pendingOAuthTransports.get(mcpName) - if (!transport) return yield* new OAuthError({ message: `No pending OAuth flow for MCP server: ${mcpName}` }) - if (!(yield* auth.consumeOAuthState(mcpName, oauthState))) { - yield* cleanupAuth(mcpName) return yield* new OAuthError({ message: "Invalid or expired OAuth state - potential CSRF attack" }) } + const transport = pendingOAuthTransports.get(mcpName) + if (!transport) { + yield* cleanupAuth(mcpName) + return yield* new OAuthError({ message: `No pending OAuth flow for MCP server: ${mcpName}` }) + } + const result = yield* Effect.tryPromise({ try: () => transport.finishAuth(authorizationCode).then(() => true as const), catch: (error) => { diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts index e4bae1a0daa8..99e7ec728313 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts @@ -88,7 +88,7 @@ export const McpApi = HttpApi.make("mcp") identifier: "mcp.auth.callback", summary: "Complete MCP OAuth", description: - "Complete OAuth authentication for a Model Context Protocol (MCP) server using the authorization code.", + "Complete OAuth authentication for a Model Context Protocol (MCP) server using the authorization code and state returned by the start endpoint.", }), ), HttpApiEndpoint.post("authAuthenticate", McpPaths.authAuthenticate, { diff --git a/packages/opencode/test/server/httpapi-mcp-oauth.test.ts b/packages/opencode/test/server/httpapi-mcp-oauth.test.ts index d0f8690fdda8..b45ca22d1a68 100644 --- a/packages/opencode/test/server/httpapi-mcp-oauth.test.ts +++ b/packages/opencode/test/server/httpapi-mcp-oauth.test.ts @@ -2,7 +2,7 @@ import { NodeHttpServer, NodeServices } from "@effect/platform-node" import Http from "node:http" import path from "node:path" import { describe, expect } from "bun:test" -import { Config, Context, Effect, Layer } from "effect" +import { Config, Context, Effect, Layer, Ref } from "effect" import { HttpClient, HttpClientRequest, @@ -32,7 +32,7 @@ const it = testEffect( const callbackPath = McpPaths.authCallback.replace(":name", "secure-oauth") const authPath = McpPaths.auth.replace(":name", "secure-oauth") -function listenOAuthServer(tokenCalls: { value: number }) { +function listenOAuthServer(tokenCalls: Ref.Ref) { return Effect.gen(function* () { const context = yield* Layer.build(NodeHttpServer.layer(Http.createServer, { host: "127.0.0.1", port: 0 })) const server = Context.get(context, HttpServer.HttpServer) @@ -57,7 +57,7 @@ function listenOAuthServer(tokenCalls: { value: number }) { if (url.pathname === "/token") return Effect.gen(function* () { yield* request.text - tokenCalls.value++ + yield* Ref.update(tokenCalls, (value) => value + 1) return yield* HttpServerResponse.json({ access_token: "access-token", token_type: "Bearer" }) }) if (url.pathname !== "/mcp") return Effect.succeed(HttpServerResponse.empty({ status: 404 })) @@ -130,7 +130,7 @@ function assertPortAvailable(port: number) { function setup() { return Effect.gen(function* () { const directory = yield* tmpdirScoped({ git: true }) - const tokenCalls = { value: 0 } + const tokenCalls = yield* Ref.make(0) const upstream = yield* listenOAuthServer(tokenCalls) const callbackPort = yield* availablePort() yield* Effect.promise(() => @@ -169,24 +169,19 @@ describe("mcp HttpApi OAuth", () => { const missing = yield* request(test.directory, callbackPath, { code: "missing-state" }) expect(missing.status).toBe(400) - expect(test.tokenCalls.value).toBe(0) + expect(yield* Ref.get(test.tokenCalls)).toBe(0) const wrong = yield* request(test.directory, callbackPath, { code: "wrong-state", state: "wrong" }) expect(wrong.status).toBe(400) - expect(test.tokenCalls.value).toBe(0) + expect(yield* Ref.get(test.tokenCalls)).toBe(0) - const restarted = yield* request(test.directory, authPath) - expect(restarted.status).toBe(200) - const second = (yield* restarted.json) as { oauthState: string } - expect(second.oauthState).not.toBe(first.oauthState) - - const correct = yield* request(test.directory, callbackPath, { code: "valid-code", state: second.oauthState }) + const correct = yield* request(test.directory, callbackPath, { code: "valid-code", state: first.oauthState }) expect(correct.status).toBe(200) - expect(test.tokenCalls.value).toBe(1) + expect(yield* Ref.get(test.tokenCalls)).toBe(1) - const replayed = yield* request(test.directory, callbackPath, { code: "replayed-code", state: second.oauthState }) + const replayed = yield* request(test.directory, callbackPath, { code: "replayed-code", state: first.oauthState }) expect(replayed.status).toBe(400) - expect(test.tokenCalls.value).toBe(1) + expect(yield* Ref.get(test.tokenCalls)).toBe(1) }), ) diff --git a/packages/opencode/test/server/httpapi-mcp.test.ts b/packages/opencode/test/server/httpapi-mcp.test.ts index 42a4398ba720..a24d2524b6ab 100644 --- a/packages/opencode/test/server/httpapi-mcp.test.ts +++ b/packages/opencode/test/server/httpapi-mcp.test.ts @@ -199,7 +199,11 @@ describe("mcp HttpApi", () => { for (const input of [ { method: "POST", route: "/mcp/missing/auth" }, { method: "POST", route: "/mcp/missing/auth/authenticate" }, - { method: "POST", route: "/mcp/missing/auth/callback", body: JSON.stringify({ code: "code" }) }, + { + method: "POST", + route: "/mcp/missing/auth/callback", + body: JSON.stringify({ code: "code", state: "state" }), + }, { method: "DELETE", route: "/mcp/missing/auth" }, { method: "POST", route: "/mcp/missing/connect" }, { method: "POST", route: "/mcp/missing/disconnect" }, diff --git a/packages/sdk/js/src/v2/gen/sdk.gen.ts b/packages/sdk/js/src/v2/gen/sdk.gen.ts index 0a09ff3d4de3..e7eeb58bf733 100644 --- a/packages/sdk/js/src/v2/gen/sdk.gen.ts +++ b/packages/sdk/js/src/v2/gen/sdk.gen.ts @@ -2296,7 +2296,7 @@ export class Auth2 extends HeyApiClient { /** * Complete MCP OAuth * - * Complete OAuth authentication for a Model Context Protocol (MCP) server using the authorization code. + * Complete OAuth authentication for a Model Context Protocol (MCP) server using the authorization code and state returned by the start endpoint. */ public callback( parameters: {