From 7ef99ac60f450d8a032d5adc709075ccf9eeb6b3 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Fri, 17 Apr 2026 15:26:35 +0300 Subject: [PATCH] refactor(sdk): remove UX policy from MCP OAuth handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strip user-facing I/O out of the SDK's OAuth surface so library, daemon, and web-app embedders are not surprised by port binds or browser opens. - DefaultMCPAuthHandler no longer calls openBrowser; it exposes an OnAuthURL(serverName, authURL) hook and performs no presentation I/O. - kit.New no longer auto-constructs a default handler when Options.MCPAuthHandler is nil. OAuth is opt-in; remote MCP servers requiring authorization fail with a clear error if no handler is set. - CLIMCPAuthHandler owns the CLI policy (browser open + stderr prints) by wiring an OnAuthURL closure on the inner DefaultMCPAuthHandler. - openBrowser is now unexported and colocated with its sole caller; no new exported helper is added to the SDK surface. BREAKING CHANGE: SDK consumers relying on implicit OAuth with a nil MCPAuthHandler must now pass kit.NewCLIMCPAuthHandler() (or a custom implementation) explicitly. The kit CLI is unaffected — cmd/root.go already constructs the handler explicitly. --- pkg/kit/kit.go | 41 +++++++++++--------- pkg/kit/oauth.go | 90 ++++++++++++++++++++++---------------------- pkg/kit/oauth_cli.go | 62 ++++++++++++++++++++++-------- 3 files changed, 116 insertions(+), 77 deletions(-) diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index d880deb2..1794457a 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -980,15 +980,23 @@ type Options struct { Debug bool // MCPAuthHandler handles OAuth authorization for remote MCP servers. - // When set, remote transports (streamable HTTP, SSE) are configured with - // OAuth support. If the server returns a 401, the handler is invoked to - // let the user authorize via browser. + // When set, remote transports (streamable HTTP, SSE) are configured + // with OAuth support. If the server returns a 401, the handler is + // invoked to let the user authorize. // - // If nil, a [DefaultMCPAuthHandler] is created automatically — opening the - // system browser and listening on a local callback server. + // If nil, OAuth is disabled: remote MCP servers requiring authorization + // will fail to connect and the underlying authorization-required error + // is surfaced to the caller. The SDK deliberately does not construct a + // default handler — doing so would bind a local TCP port and trigger + // presentation I/O (browser open, stderr writes) without the consumer + // opting in, which is wrong for library, daemon, or web-app embedders. // - // Set to a custom implementation to control the authorization UX (e.g. - // display a URL in a custom UI, redirect to a web app, etc.). + // CLI consumers: pass [NewCLIMCPAuthHandler] to get the standard + // "open browser + print status" behavior. + // + // Custom UX: implement [MCPAuthHandler] directly, or use + // [DefaultMCPAuthHandler] and set its OnAuthURL hook to plug in your + // own presentation (TUI modal, QR code, web redirect, etc.). MCPAuthHandler MCPAuthHandler // MCPTokenStoreFactory, if non-nil, is called to create a token store for @@ -1362,20 +1370,19 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { OnMCPServerLoaded: opts.OnMCPServerLoaded, } - // Set up OAuth handler for remote MCP servers. + // Set up OAuth handler for remote MCP servers. The SDK does not create + // a default handler: auto-construction would bind a local TCP port and + // (historically) shell out to a browser without the consumer asking, + // which is a surprise for library/daemon/web-app embedders. Consumers + // that want CLI behavior pass a [CLIMCPAuthHandler] explicitly; other + // consumers implement [MCPAuthHandler] themselves. If nil, remote MCP + // servers requiring OAuth will fail to connect with the underlying + // authorization-required error surfaced to the caller. + // // The SDK MCPAuthHandler interface is structurally identical to // tools.MCPAuthHandler, so any implementation satisfies both. if opts.MCPAuthHandler != nil { setupOpts.AuthHandler = opts.MCPAuthHandler - } else { - // Create a default handler that opens the system browser. - defaultHandler, authErr := NewDefaultMCPAuthHandler() - if authErr != nil { - // Non-fatal: OAuth just won't be available for remote servers. - log.Printf("WARN Failed to create OAuth handler; remote MCP servers requiring auth will fail: %v", authErr) - } else { - setupOpts.AuthHandler = defaultHandler - } } // Set up custom token store factory for MCP OAuth tokens. diff --git a/pkg/kit/oauth.go b/pkg/kit/oauth.go index 07991d33..82d62ded 100644 --- a/pkg/kit/oauth.go +++ b/pkg/kit/oauth.go @@ -5,18 +5,18 @@ import ( "fmt" "net" "net/http" - "os/exec" - "runtime" "sync" "time" ) // MCPAuthHandler handles OAuth authorization for MCP servers. // Implementations control the user experience — opening a browser, showing a -// prompt, displaying a URL, etc. +// prompt, displaying a URL, posting to a message bus, etc. // -// The default implementation ([DefaultMCPAuthHandler]) opens the system browser -// and starts a local HTTP callback server to receive the authorization code. +// [DefaultMCPAuthHandler] provides the transport mechanics (port reservation +// and callback server) but performs no user-facing I/O on its own; consumers +// wire presentation via [DefaultMCPAuthHandler.OnAuthURL] or implement +// MCPAuthHandler from scratch. type MCPAuthHandler interface { // RedirectURI returns the OAuth redirect URI that the callback server // will listen on. This is called during MCP transport setup — before any @@ -37,23 +37,44 @@ type MCPAuthHandler interface { HandleAuth(ctx context.Context, serverName string, authURL string) (callbackURL string, err error) } -// DefaultMCPAuthHandler opens the system browser and starts a local HTTP -// callback server to receive the OAuth authorization code. It eagerly reserves -// a TCP port on construction so [RedirectURI] is stable for the lifetime of -// the handler. +// DefaultMCPAuthHandler provides the transport mechanics of an OAuth flow — +// reserving a local TCP port and running a one-shot HTTP callback server — +// without making any user-experience decisions. It performs no browser opens, +// no printing, no TUI calls; consumers attach presentation by setting +// [DefaultMCPAuthHandler.OnAuthURL] or by wrapping the handler. // -// Create instances with [NewDefaultMCPAuthHandler] (random port) or -// [NewDefaultMCPAuthHandlerWithPort] (explicit port). +// The handler eagerly reserves a TCP port on construction so [RedirectURI] is +// stable for the lifetime of the handler. Create instances with +// [NewDefaultMCPAuthHandler] (random port) or [NewDefaultMCPAuthHandlerWithPort] +// (explicit port). Always call [DefaultMCPAuthHandler.Close] when done to +// release the port. type DefaultMCPAuthHandler struct { listener net.Listener port int mu sync.Mutex // guards listener lifecycle + + // OnAuthURL, if set, is invoked exactly once per [HandleAuth] call with + // the authorization URL the user must visit. This is where consumers + // plug in their UX: open a browser, print to stderr, post to a TUI + // stream, render a QR code, etc. The handler performs no I/O on the + // URL itself; if OnAuthURL is nil the URL is silently dropped and the + // user has no way to complete the flow. + // + // OnAuthURL is called synchronously before the handler blocks on the + // callback. It must not block indefinitely — long-running work should + // be dispatched to a goroutine. + OnAuthURL func(serverName, authURL string) } // NewDefaultMCPAuthHandler creates a handler that listens on a random // available port on localhost. The port is reserved immediately so // [RedirectURI] returns a stable value. Call [DefaultMCPAuthHandler.Close] // when the handler is no longer needed to release the port. +// +// The returned handler has no OnAuthURL hook configured and will therefore +// appear to hang on HandleAuth until the context deadline fires. Set +// OnAuthURL before using the handler, or use a higher-level wrapper such +// as [CLIMCPAuthHandler]. func NewDefaultMCPAuthHandler() (*DefaultMCPAuthHandler, error) { listener, err := net.Listen("tcp", "localhost:0") if err != nil { @@ -88,9 +109,9 @@ func (h *DefaultMCPAuthHandler) Port() int { return h.port } -// HandleAuth opens the system browser to authURL and waits for the OAuth -// callback on the local server. It returns the full callback URL including -// query parameters (code, state, etc.). +// HandleAuth invokes [OnAuthURL] with the authorization URL (if configured) +// and waits for the OAuth callback on the local server. It returns the full +// callback URL including query parameters (code, state, etc.). // // If the context has no deadline, a default 2-minute timeout is applied. // The callback server is started for each HandleAuth call and shut down @@ -136,19 +157,13 @@ func (h *DefaultMCPAuthHandler) HandleAuth(ctx context.Context, serverName strin Handler: mux, } - // Start serving on the pre-reserved listener. We need to create a new - // listener on the same port because http.Server.Serve takes ownership - // and closes the listener when done. The original listener is kept open - // to reserve the port; we create a second listener via SO_REUSEADDR - // semantics (Go's default on most platforms) or, more reliably, we - // temporarily release and re-acquire. - // - // Strategy: use the held listener directly for Serve. After Serve - // returns (due to Shutdown), re-acquire the listener to keep the port - // reserved for future HandleAuth calls. + // Start serving on the pre-reserved listener. http.Server.Serve takes + // ownership and closes the listener when Shutdown is called, so we + // re-acquire a fresh listener on the same port in the deferred cleanup + // below to keep the port reserved for subsequent HandleAuth calls. h.mu.Lock() serveListener := h.listener - h.listener = nil // Serve will close it + h.listener = nil h.mu.Unlock() if serveListener == nil { @@ -184,10 +199,11 @@ func (h *DefaultMCPAuthHandler) HandleAuth(ctx context.Context, serverName strin } }() - // Open the system browser. - if err := openBrowser(authURL); err != nil { - // Browser open is best-effort; the user can still navigate manually. - _ = err + // Surface the authorization URL to the consumer. This is the single + // presentation seam: the SDK itself does not open browsers, print, + // or otherwise touch the user's environment. + if h.OnAuthURL != nil { + h.OnAuthURL(serverName, authURL) } // Wait for the callback, a server error, or context cancellation. @@ -214,22 +230,6 @@ func (h *DefaultMCPAuthHandler) Close() error { return nil } -// openBrowser opens the default system browser to the given URL. This is a -// best-effort operation — errors are returned but callers typically ignore -// them since the user can navigate manually. -func openBrowser(url string) error { - switch runtime.GOOS { - case "linux": - return exec.Command("xdg-open", url).Start() - case "windows": - return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start() - case "darwin": - return exec.Command("open", url).Start() - default: - return fmt.Errorf("unsupported platform: %s", runtime.GOOS) - } -} - // oauthSuccessHTML is the HTML page returned to the browser after a // successful OAuth callback. const oauthSuccessHTML = ` diff --git a/pkg/kit/oauth_cli.go b/pkg/kit/oauth_cli.go index f9c956fc..36eaaec5 100644 --- a/pkg/kit/oauth_cli.go +++ b/pkg/kit/oauth_cli.go @@ -5,32 +5,49 @@ import ( "fmt" "io" "os" + "os/exec" + "runtime" ) -// CLIMCPAuthHandler wraps a [DefaultMCPAuthHandler] and prints status messages -// to a writer (typically stderr) so the user knows what's happening during -// OAuth authorization. This is the handler used by the CLI/TUI binary. +// CLIMCPAuthHandler is the MCP OAuth handler for CLI/TUI consumers. It wraps +// a [DefaultMCPAuthHandler] and layers standard CLI behavior on top of the +// underlying transport mechanics: // -// For TUI integration, set NotifyFunc to route messages through the TUI's -// event system instead of (or in addition to) the writer. +// - Opens the authorization URL in the system browser +// - Prints status messages (or routes them to a TUI via [NotifyFunc]) +// +// Non-CLI consumers (web apps, daemons, custom TUIs) should not use this +// handler; implement [MCPAuthHandler] directly or configure a +// [DefaultMCPAuthHandler] with a custom OnAuthURL instead. type CLIMCPAuthHandler struct { inner *DefaultMCPAuthHandler w io.Writer - // NotifyFunc, when set, is called with status messages instead of writing - // to the writer. This allows the TUI to display system messages in the - // chat stream. If nil, messages are written to w. + // NotifyFunc, when set, is called with status messages instead of + // writing to the writer. This allows the TUI to display system + // messages in the chat stream. If nil, messages are written to w. NotifyFunc func(serverName, message string) } // NewCLIMCPAuthHandler creates a CLI auth handler that prints status messages -// to stderr and delegates the actual OAuth flow to a [DefaultMCPAuthHandler]. +// to stderr, opens the authorization URL in the system browser, and delegates +// the callback-server mechanics to a [DefaultMCPAuthHandler]. func NewCLIMCPAuthHandler() (*CLIMCPAuthHandler, error) { inner, err := NewDefaultMCPAuthHandler() if err != nil { return nil, err } - return &CLIMCPAuthHandler{inner: inner, w: os.Stderr}, nil + h := &CLIMCPAuthHandler{inner: inner, w: os.Stderr} + // Wire the CLI presentation policy into the inner handler's hook. + // This is the one place in the codebase where OAuth triggers a + // browser open; the SDK core remains I/O-free. + inner.OnAuthURL = func(serverName, authURL string) { + h.notify(serverName, fmt.Sprintf("🔐 MCP server %q requires authentication. Opening browser...", serverName)) + h.notify(serverName, fmt.Sprintf(" If the browser doesn't open, visit:\n %s", authURL)) + // Browser open is best-effort; the user can still navigate manually. + _ = openBrowser(authURL) + } + return h, nil } // RedirectURI returns the OAuth redirect URI from the inner handler. @@ -38,17 +55,15 @@ func (h *CLIMCPAuthHandler) RedirectURI() string { return h.inner.RedirectURI() } -// HandleAuth prints status messages and delegates to the inner handler. +// HandleAuth delegates to the inner handler (which invokes OnAuthURL, runs +// the callback server, and returns the full callback URL) and emits a final +// success or failure notification. func (h *CLIMCPAuthHandler) HandleAuth(ctx context.Context, serverName string, authURL string) (string, error) { - h.notify(serverName, fmt.Sprintf("🔐 MCP server %q requires authentication. Opening browser...", serverName)) - h.notify(serverName, fmt.Sprintf(" If the browser doesn't open, visit:\n %s", authURL)) - callbackURL, err := h.inner.HandleAuth(ctx, serverName, authURL) if err != nil { h.notify(serverName, fmt.Sprintf("✗ Authentication failed for %q: %v", serverName, err)) return "", err } - h.notify(serverName, fmt.Sprintf("✓ Authenticated with %q", serverName)) return callbackURL, nil } @@ -66,3 +81,20 @@ func (h *CLIMCPAuthHandler) notify(serverName, message string) { } _, _ = fmt.Fprintln(h.w, message) } + +// openBrowser opens the system default browser at url. Intentionally +// unexported: browser opening is CLI policy, not SDK surface. Consumers +// that need similar behavior for their own UX should bring their own +// helper (or use a third-party package like github.com/pkg/browser). +func openBrowser(url string) error { + switch runtime.GOOS { + case "linux": + return exec.Command("xdg-open", url).Start() + case "windows": + return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start() + case "darwin": + return exec.Command("open", url).Start() + default: + return fmt.Errorf("unsupported platform: %s", runtime.GOOS) + } +}