diff --git a/packages/opencode/src/mcp/auth.ts b/packages/opencode/src/mcp/auth.ts index cd829dd38d..d48ebc0d1e 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 90b136a8e8..f0a1e616a0 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 e4bae1a0da..99e7ec7283 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 d0f8690fdd..b45ca22d1a 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 42a4398ba7..a24d2524b6 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 0a09ff3d4d..e7eeb58bf7 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: {