Browse Source

Revert "feat(mcp): add OAuth redirect URI configuration for MCP servers (#7379)"

This reverts commit 40b275d7e6759d2209bbaec7d4d13f729198bda3.
Aiden Cline 2 months ago
parent
commit
33290c54cd

+ 0 - 6
packages/opencode/src/cli/cmd/mcp.ts

@@ -6,7 +6,6 @@ import * as prompts from "@clack/prompts"
 import { UI } from "../ui"
 import { MCP } from "../../mcp"
 import { McpAuth } from "../../mcp/auth"
-import { McpOAuthCallback } from "../../mcp/oauth-callback"
 import { McpOAuthProvider } from "../../mcp/oauth-provider"
 import { Config } from "../../config/config"
 import { Instance } from "../../project/instance"
@@ -683,10 +682,6 @@ export const McpDebugCommand = cmd({
 
             // Try to discover OAuth metadata
             const oauthConfig = typeof serverConfig.oauth === "object" ? serverConfig.oauth : undefined
-
-            // Start callback server
-            await McpOAuthCallback.ensureRunning(oauthConfig?.redirectUri)
-
             const authProvider = new McpOAuthProvider(
               serverName,
               serverConfig.url,
@@ -694,7 +689,6 @@ export const McpDebugCommand = cmd({
                 clientId: oauthConfig?.clientId,
                 clientSecret: oauthConfig?.clientSecret,
                 scope: oauthConfig?.scope,
-                redirectUri: oauthConfig?.redirectUri,
               },
               {
                 onRedirect: async () => {},

+ 0 - 4
packages/opencode/src/config/config.ts

@@ -435,10 +435,6 @@ export namespace Config {
         .describe("OAuth client ID. If not provided, dynamic client registration (RFC 7591) will be attempted."),
       clientSecret: z.string().optional().describe("OAuth client secret (if required by the authorization server)"),
       scope: z.string().optional().describe("OAuth scopes to request during authorization"),
-      redirectUri: z
-        .string()
-        .optional()
-        .describe("OAuth redirect URI (default: http://127.0.0.1:19876/mcp/oauth/callback)."),
     })
     .strict()
     .meta({

+ 10 - 17
packages/opencode/src/mcp/index.ts

@@ -308,8 +308,6 @@ export namespace MCP {
       let authProvider: McpOAuthProvider | undefined
 
       if (!oauthDisabled) {
-        await McpOAuthCallback.ensureRunning(oauthConfig?.redirectUri)
-
         authProvider = new McpOAuthProvider(
           key,
           mcp.url,
@@ -317,7 +315,6 @@ export namespace MCP {
             clientId: oauthConfig?.clientId,
             clientSecret: oauthConfig?.clientSecret,
             scope: oauthConfig?.scope,
-            redirectUri: oauthConfig?.redirectUri,
           },
           {
             onRedirect: async (url) => {
@@ -347,7 +344,6 @@ export namespace MCP {
 
       let lastError: Error | undefined
       const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT
-
       for (const { name, transport } of transports) {
         try {
           const client = new Client({
@@ -574,8 +570,7 @@ export namespace MCP {
 
     for (const [clientName, client] of Object.entries(clientsSnapshot)) {
       // Only include tools from connected MCPs (skip disabled ones)
-      const clientStatus = s.status[clientName]?.status
-      if (clientStatus !== "connected") {
+      if (s.status[clientName]?.status !== "connected") {
         continue
       }
 
@@ -725,10 +720,8 @@ export namespace MCP {
       throw new Error(`MCP server ${mcpName} has OAuth explicitly disabled`)
     }
 
-    // OAuth config is optional - if not provided, we'll use auto-discovery
-    const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined
-
-    await McpOAuthCallback.ensureRunning(oauthConfig?.redirectUri)
+    // Start the callback server
+    await McpOAuthCallback.ensureRunning()
 
     // Generate and store a cryptographically secure state parameter BEFORE creating the provider
     // The SDK will call provider.state() to read this value
@@ -738,6 +731,8 @@ export namespace MCP {
     await McpAuth.updateOAuthState(mcpName, oauthState)
 
     // Create a new auth provider for this flow
+    // OAuth config is optional - if not provided, we'll use auto-discovery
+    const oauthConfig = typeof mcpConfig.oauth === "object" ? mcpConfig.oauth : undefined
     let capturedUrl: URL | undefined
     const authProvider = new McpOAuthProvider(
       mcpName,
@@ -746,7 +741,6 @@ export namespace MCP {
         clientId: oauthConfig?.clientId,
         clientSecret: oauthConfig?.clientSecret,
         scope: oauthConfig?.scope,
-        redirectUri: oauthConfig?.redirectUri,
       },
       {
         onRedirect: async (url) => {
@@ -775,7 +769,6 @@ export namespace MCP {
         pendingOAuthTransports.set(mcpName, transport)
         return { authorizationUrl: capturedUrl.toString() }
       }
-
       throw error
     }
   }
@@ -785,9 +778,9 @@ export namespace MCP {
    * Opens the browser and waits for callback.
    */
   export async function authenticate(mcpName: string): Promise<Status> {
-    const result = await startAuth(mcpName)
+    const { authorizationUrl } = await startAuth(mcpName)
 
-    if (!result.authorizationUrl) {
+    if (!authorizationUrl) {
       // Already authenticated
       const s = await state()
       return s.status[mcpName] ?? { status: "connected" }
@@ -801,9 +794,9 @@ export namespace MCP {
 
     // The SDK has already added the state parameter to the authorization URL
     // We just need to open the browser
-    log.info("opening browser for oauth", { mcpName, url: result.authorizationUrl, state: oauthState })
+    log.info("opening browser for oauth", { mcpName, url: authorizationUrl, state: oauthState })
     try {
-      const subprocess = await open(result.authorizationUrl)
+      const subprocess = await open(authorizationUrl)
       // The open package spawns a detached process and returns immediately.
       // We need to listen for errors which fire asynchronously:
       // - "error" event: command not found (ENOENT)
@@ -826,7 +819,7 @@ export namespace MCP {
       // Browser opening failed (e.g., in remote/headless sessions like SSH, devcontainers)
       // Emit event so CLI can display the URL for manual opening
       log.warn("failed to open browser, user must open URL manually", { mcpName, error })
-      Bus.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl })
+      Bus.publish(BrowserOpenFailed, { mcpName, url: authorizationUrl })
     }
 
     // Wait for callback using the OAuth state parameter

+ 9 - 25
packages/opencode/src/mcp/oauth-callback.ts

@@ -1,12 +1,8 @@
 import { Log } from "../util/log"
-import { OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH, parseRedirectUri } from "./oauth-provider"
+import { OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH } from "./oauth-provider"
 
 const log = Log.create({ service: "mcp.oauth-callback" })
 
-// Current callback server configuration (may differ from defaults if custom redirectUri is used)
-let currentPort = OAUTH_CALLBACK_PORT
-let currentPath = OAUTH_CALLBACK_PATH
-
 const HTML_SUCCESS = `<!DOCTYPE html>
 <html>
 <head>
@@ -60,33 +56,21 @@ export namespace McpOAuthCallback {
 
   const CALLBACK_TIMEOUT_MS = 5 * 60 * 1000 // 5 minutes
 
-  export async function ensureRunning(redirectUri?: string): Promise<void> {
-    // Parse the redirect URI to get port and path (uses defaults if not provided)
-    const { port, path } = parseRedirectUri(redirectUri)
-
-    // If server is running on a different port/path, stop it first
-    if (server && (currentPort !== port || currentPath !== path)) {
-      log.info("stopping oauth callback server to reconfigure", { oldPort: currentPort, newPort: port })
-      await stop()
-    }
-
+  export async function ensureRunning(): Promise<void> {
     if (server) return
 
-    const running = await isPortInUse(port)
+    const running = await isPortInUse()
     if (running) {
-      log.info("oauth callback server already running on another instance", { port })
+      log.info("oauth callback server already running on another instance", { port: OAUTH_CALLBACK_PORT })
       return
     }
 
-    currentPort = port
-    currentPath = path
-
     server = Bun.serve({
-      port: currentPort,
+      port: OAUTH_CALLBACK_PORT,
       fetch(req) {
         const url = new URL(req.url)
 
-        if (url.pathname !== currentPath) {
+        if (url.pathname !== OAUTH_CALLBACK_PATH) {
           return new Response("Not found", { status: 404 })
         }
 
@@ -149,7 +133,7 @@ export namespace McpOAuthCallback {
       },
     })
 
-    log.info("oauth callback server started", { port: currentPort, path: currentPath })
+    log.info("oauth callback server started", { port: OAUTH_CALLBACK_PORT })
   }
 
   export function waitForCallback(oauthState: string): Promise<string> {
@@ -174,11 +158,11 @@ export namespace McpOAuthCallback {
     }
   }
 
-  export async function isPortInUse(port: number = OAUTH_CALLBACK_PORT): Promise<boolean> {
+  export async function isPortInUse(): Promise<boolean> {
     return new Promise((resolve) => {
       Bun.connect({
         hostname: "127.0.0.1",
-        port,
+        port: OAUTH_CALLBACK_PORT,
         socket: {
           open(socket) {
             socket.end()

+ 0 - 24
packages/opencode/src/mcp/oauth-provider.ts

@@ -17,7 +17,6 @@ export interface McpOAuthConfig {
   clientId?: string
   clientSecret?: string
   scope?: string
-  redirectUri?: string
 }
 
 export interface McpOAuthCallbacks {
@@ -33,10 +32,6 @@ export class McpOAuthProvider implements OAuthClientProvider {
   ) {}
 
   get redirectUrl(): string {
-    // Use configured redirectUri if provided, otherwise use OpenCode defaults
-    if (this.config.redirectUri) {
-      return this.config.redirectUri
-    }
     return `http://127.0.0.1:${OAUTH_CALLBACK_PORT}${OAUTH_CALLBACK_PATH}`
   }
 
@@ -157,22 +152,3 @@ export class McpOAuthProvider implements OAuthClientProvider {
 }
 
 export { OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH }
-
-/**
- * Parse a redirect URI to extract port and path for the callback server.
- * Returns defaults if the URI can't be parsed.
- */
-export function parseRedirectUri(redirectUri?: string): { port: number; path: string } {
-  if (!redirectUri) {
-    return { port: OAUTH_CALLBACK_PORT, path: OAUTH_CALLBACK_PATH }
-  }
-
-  try {
-    const url = new URL(redirectUri)
-    const port = url.port ? parseInt(url.port, 10) : url.protocol === "https:" ? 443 : 80
-    const path = url.pathname || OAUTH_CALLBACK_PATH
-    return { port, path }
-  } catch {
-    return { port: OAUTH_CALLBACK_PORT, path: OAUTH_CALLBACK_PATH }
-  }
-}

+ 0 - 75
packages/opencode/test/mcp/oauth-callback.test.ts

@@ -1,75 +0,0 @@
-import { test, expect, describe, afterEach } from "bun:test"
-import { McpOAuthCallback } from "../../src/mcp/oauth-callback"
-import { parseRedirectUri } from "../../src/mcp/oauth-provider"
-
-describe("McpOAuthCallback.ensureRunning", () => {
-  afterEach(async () => {
-    await McpOAuthCallback.stop()
-  })
-
-  test("starts server with default config when no redirectUri provided", async () => {
-    await McpOAuthCallback.ensureRunning()
-    expect(McpOAuthCallback.isRunning()).toBe(true)
-  })
-
-  test("starts server with custom redirectUri", async () => {
-    await McpOAuthCallback.ensureRunning("http://127.0.0.1:18000/custom/callback")
-    expect(McpOAuthCallback.isRunning()).toBe(true)
-  })
-
-  test("is idempotent when called with same redirectUri", async () => {
-    await McpOAuthCallback.ensureRunning("http://127.0.0.1:18001/callback")
-    await McpOAuthCallback.ensureRunning("http://127.0.0.1:18001/callback")
-    expect(McpOAuthCallback.isRunning()).toBe(true)
-  })
-
-  test("restarts server when redirectUri changes", async () => {
-    await McpOAuthCallback.ensureRunning("http://127.0.0.1:18002/path1")
-    expect(McpOAuthCallback.isRunning()).toBe(true)
-
-    await McpOAuthCallback.ensureRunning("http://127.0.0.1:18003/path2")
-    expect(McpOAuthCallback.isRunning()).toBe(true)
-  })
-
-  test("isRunning returns false when not started", async () => {
-    expect(McpOAuthCallback.isRunning()).toBe(false)
-  })
-
-  test("isRunning returns false after stop", async () => {
-    await McpOAuthCallback.ensureRunning()
-    await McpOAuthCallback.stop()
-    expect(McpOAuthCallback.isRunning()).toBe(false)
-  })
-})
-
-describe("parseRedirectUri", () => {
-  test("returns defaults when no URI provided", () => {
-    const result = parseRedirectUri()
-    expect(result.port).toBe(19876)
-    expect(result.path).toBe("/mcp/oauth/callback")
-  })
-
-  test("parses port and path from URI", () => {
-    const result = parseRedirectUri("http://127.0.0.1:8080/oauth/callback")
-    expect(result.port).toBe(8080)
-    expect(result.path).toBe("/oauth/callback")
-  })
-
-  test("defaults to port 80 for http without explicit port", () => {
-    const result = parseRedirectUri("http://127.0.0.1/callback")
-    expect(result.port).toBe(80)
-    expect(result.path).toBe("/callback")
-  })
-
-  test("defaults to port 443 for https without explicit port", () => {
-    const result = parseRedirectUri("https://127.0.0.1/callback")
-    expect(result.port).toBe(443)
-    expect(result.path).toBe("/callback")
-  })
-
-  test("returns defaults for invalid URI", () => {
-    const result = parseRedirectUri("not-a-valid-url")
-    expect(result.port).toBe(19876)
-    expect(result.path).toBe("/mcp/oauth/callback")
-  })
-})