fix(opencode): secure manual MCP OAuth callback

packages/opencode/src/mcp/auth.ts

@@ -49,6 +49,7 @@ export interface Interface {
   readonly clearCodeVerifier: (mcpName: string) => Effect.Effect<void>
   readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect<void>
   readonly getOAuthState: (mcpName: string) => Effect.Effect<string | undefined>
+  readonly consumeOAuthState: (mcpName: string, oauthState: string) => Effect.Effect<boolean>
   readonly clearOAuthState: (mcpName: string) => Effect.Effect<void>
   readonly isTokenExpired: (mcpName: string) => Effect.Effect<boolean | null>
 }
@@ -142,6 +143,17 @@ 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 !== oauthState) return false
+        delete entry.oauthState
+        yield* fs.writeJson(filepath, { ...data, [mcpName]: entry }, 0o600).pipe(Effect.orDie)
+        return true
+      }).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 +173,7 @@ export const layer = Layer.effect(
       clearCodeVerifier,
       updateOAuthState,
       getOAuthState,
+      consumeOAuthState,
       clearOAuthState,
       isTokenExpired,
     })

packages/opencode/src/mcp/index.ts

@@ -71,6 +71,10 @@ export class NotFoundError extends Schema.TaggedErrorClass<NotFoundError>()("MCP
   name: Schema.String,
 }) {}
 
+export class OAuthError extends Schema.TaggedErrorClass<OAuthError>()("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<Status, NotFoundError>
-  readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect<Status, NotFoundError>
+  readonly finishAuth: (
+    mcpName: string,
+    authorizationCode: string,
+    oauthState: string,
+  ) => Effect.Effect<Status, NotFoundError | OAuthError>
   readonly removeAuth: (mcpName: string) => Effect.Effect<void>
   readonly supportsOAuth: (mcpName: string) => Effect.Effect<boolean, NotFoundError>
   readonly hasStoredTokens: (mcpName: string) => Effect.Effect<boolean>
@@ -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,68 @@ 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<void, Error>((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<void, Error>((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)
+      if (!(yield* auth.consumeOAuthState(mcpName, oauthState))) {
+        return yield* new OAuthError({ message: "Invalid or expired OAuth state - potential CSRF attack" })
+      }
+
       const transport = pendingOAuthTransports.get(mcpName)
-      if (!transport) throw new Error(`No pending OAuth flow for MCP server: ${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),
@@ -927,13 +968,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)

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),
@@ -87,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, {

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 } }) {