From 3a74e408fe100b5c8b07e8a458b2ee7bf25d4450 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 21:44:05 -0400 Subject: [PATCH 1/3] fix(mcp): skip RFC 8414 discovery when OAuth endpoints are preconfigured resolveAuthorizationServer performed a real network metadata discovery call even when the MCP server config already supplied both the authorization and token endpoints. When discovery blocks (offline, unreachable issuer, or test fixtures pointing at invalid hosts) the whole Login flow hangs and never reaches the browser/loopback step, so mcp oauth login could not complete. When both endpoints are present, return them directly and skip discovery entirely; discovery remains the fallback for any endpoint the config leaves blank. This also fixes TestRunMCPOAuthLoginStoresTokens, which timed out because the loopback callback was never driven (the URL was never printed). Removes scratch test files left over from investigation (internal/mcp/scratch_cli_test.go, internal/cli/scratch_oauth_test.go). --- internal/mcp/oauth.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/mcp/oauth.go b/internal/mcp/oauth.go index 9106fdf5b..b21ef31bd 100644 --- a/internal/mcp/oauth.go +++ b/internal/mcp/oauth.go @@ -137,6 +137,20 @@ func discoverAuthorizationServer(ctx context.Context, client *http.Client, baseU // overrides. Configured endpoints take precedence over discovered ones, and act // as a fallback when discovery fails or omits a value. func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseURL string, cfg OAuthConfig) (authServerMetadata, error) { + // When the config supplies both the authorization and token endpoints + // directly, skip network discovery entirely: there is nothing to discover, + // and a hung/blocked discovery call (e.g. offline, or an unreachable issuer + // in tests) must not gate an otherwise fully-specified login. Discovery stays + // the fallback for any endpoint the config leaves blank. + if strings.TrimSpace(cfg.AuthorizationEndpoint) != "" && strings.TrimSpace(cfg.TokenEndpoint) != "" { + metadata := authServerMetadata{ + AuthorizationEndpoint: cfg.AuthorizationEndpoint, + TokenEndpoint: cfg.TokenEndpoint, + RegistrationEndpoint: cfg.RegistrationEndpoint, + } + return metadata, nil + } + discoveryBase := strings.TrimSpace(cfg.IssuerURL) if discoveryBase == "" { discoveryBase = baseURL From 27b81955a21eb11d2eb0786de448402047e12110 Mon Sep 17 00:00:00 2001 From: gnanam1990 Date: Wed, 8 Jul 2026 11:39:02 +0530 Subject: [PATCH 2/3] fix: trim fast-path endpoints, restore fallback test, fix picker_test compile error - Trim AuthorizationEndpoint/TokenEndpoint/RegistrationEndpoint in the skip-discovery fast path to match the fallback path (P3 from @jatmn). - Update TestResolveEndpointsFallsBackToConfig to only configure one endpoint so discovery still runs and fails, exercising the fallback. - Add TestResolveEndpointsSkipsDiscoveryWhenBothConfigured to verify discovery is skipped when both endpoints are configured (P2 from @jatmn). - Fix picker_test.go:771 to capture 4 return values from switchProviderModel (pre-existing compile error on main blocking CI smoke). --- internal/mcp/oauth.go | 6 +++--- internal/mcp/oauth_test.go | 37 ++++++++++++++++++++++++++++++------- internal/tui/picker_test.go | 2 +- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/internal/mcp/oauth.go b/internal/mcp/oauth.go index b21ef31bd..b435fbd7f 100644 --- a/internal/mcp/oauth.go +++ b/internal/mcp/oauth.go @@ -144,9 +144,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR // the fallback for any endpoint the config leaves blank. if strings.TrimSpace(cfg.AuthorizationEndpoint) != "" && strings.TrimSpace(cfg.TokenEndpoint) != "" { metadata := authServerMetadata{ - AuthorizationEndpoint: cfg.AuthorizationEndpoint, - TokenEndpoint: cfg.TokenEndpoint, - RegistrationEndpoint: cfg.RegistrationEndpoint, + AuthorizationEndpoint: strings.TrimSpace(cfg.AuthorizationEndpoint), + TokenEndpoint: strings.TrimSpace(cfg.TokenEndpoint), + RegistrationEndpoint: strings.TrimSpace(cfg.RegistrationEndpoint), } return metadata, nil } diff --git a/internal/mcp/oauth_test.go b/internal/mcp/oauth_test.go index 610d6da44..bba6b53c0 100644 --- a/internal/mcp/oauth_test.go +++ b/internal/mcp/oauth_test.go @@ -51,7 +51,10 @@ func TestDiscoverParsesMetadata(t *testing.T) { func TestResolveEndpointsFallsBackToConfig(t *testing.T) { // Server with no metadata document: discovery must fail and the resolver must - // fall back to the explicitly configured endpoints. + // fall back to the explicitly configured authorization endpoint. Only one + // endpoint is configured so the skip-discovery fast path does not fire; + // the token endpoint comes from discovery (which fails here), so the + // resolver must return an error for the missing token endpoint. server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) })) @@ -59,17 +62,37 @@ func TestResolveEndpointsFallsBackToConfig(t *testing.T) { cfg := OAuthConfig{ AuthorizationEndpoint: "https://issuer.example/authorize", - TokenEndpoint: "https://issuer.example/token", } - meta, err := resolveAuthorizationServer(context.Background(), http.DefaultClient, server.URL, cfg) + _, err := resolveAuthorizationServer(context.Background(), http.DefaultClient, server.URL, cfg) + if err == nil { + t.Fatal("expected error for missing token endpoint when discovery fails, got nil") + } + if !strings.Contains(err.Error(), "token endpoint") { + t.Fatalf("error = %q, want it to mention token endpoint", err) + } +} + +func TestResolveEndpointsSkipsDiscoveryWhenBothConfigured(t *testing.T) { + // When both endpoints are configured, discovery must be skipped entirely. + // Point the base URL at an invalid host so any discovery call would fail + // or hang -- if the fast path works, the test completes instantly. + cfg := OAuthConfig{ + AuthorizationEndpoint: " https://issuer.example/authorize ", + TokenEndpoint: " https://issuer.example/token ", + RegistrationEndpoint: " https://issuer.example/register ", + } + meta, err := resolveAuthorizationServer(context.Background(), http.DefaultClient, "http://0.0.0.0:0/invalid", cfg) if err != nil { t.Fatalf("resolveAuthorizationServer() error = %v", err) } - if meta.AuthorizationEndpoint != cfg.AuthorizationEndpoint { - t.Fatalf("authorization endpoint = %q, want config fallback", meta.AuthorizationEndpoint) + if meta.AuthorizationEndpoint != "https://issuer.example/authorize" { + t.Fatalf("authorization endpoint = %q, want trimmed config value", meta.AuthorizationEndpoint) + } + if meta.TokenEndpoint != "https://issuer.example/token" { + t.Fatalf("token endpoint = %q, want trimmed config value", meta.TokenEndpoint) } - if meta.TokenEndpoint != cfg.TokenEndpoint { - t.Fatalf("token endpoint = %q, want config fallback", meta.TokenEndpoint) + if meta.RegistrationEndpoint != "https://issuer.example/register" { + t.Fatalf("registration endpoint = %q, want trimmed config value", meta.RegistrationEndpoint) } } diff --git a/internal/tui/picker_test.go b/internal/tui/picker_test.go index 93f144b99..e9532ef54 100644 --- a/internal/tui/picker_test.go +++ b/internal/tui/picker_test.go @@ -768,7 +768,7 @@ func TestSwitchProviderModelRecordsRecentHistory(t *testing.T) { }, }) - next, status, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + next, status, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") wantStatus := "Model\nSwitched to ollama ยท kimi-k2.7-code:cloud" if status != wantStatus { t.Fatalf("switchProviderModel() status = %q, want %q (a mismatch here means the switch itself failed, not the recentModels assertion below)", status, wantStatus) From f0bf1b6260756f11f3ec0d41e13b273dab7d5dca Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 8 Jul 2026 02:17:18 -0400 Subject: [PATCH 3/3] test(mcp): prove discovery is skipped via request-failing transport The skip-discovery test passed even without the fast path, because discovery against the invalid URL failed instantly and the config fallback produced identical trimmed endpoints. Use a transport that fails the test on any outbound request so the fast path is actually what is being verified. --- internal/mcp/oauth_test.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/internal/mcp/oauth_test.go b/internal/mcp/oauth_test.go index bba6b53c0..ddb46f567 100644 --- a/internal/mcp/oauth_test.go +++ b/internal/mcp/oauth_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/base64" "encoding/json" + "errors" "net/http" "net/http/httptest" "net/url" @@ -72,16 +73,26 @@ func TestResolveEndpointsFallsBackToConfig(t *testing.T) { } } +// roundTripperFunc adapts a function to http.RoundTripper so a test can +// observe every outbound request. +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + func TestResolveEndpointsSkipsDiscoveryWhenBothConfigured(t *testing.T) { // When both endpoints are configured, discovery must be skipped entirely. - // Point the base URL at an invalid host so any discovery call would fail - // or hang -- if the fast path works, the test completes instantly. + // The client fails any outbound request, so this test distinguishes the + // fast path from a discovery failure rescued by the config fallback. + client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) { + t.Errorf("unexpected HTTP request to %s: discovery should be skipped", r.URL) + return nil, errors.New("discovery must be skipped") + })} cfg := OAuthConfig{ AuthorizationEndpoint: " https://issuer.example/authorize ", TokenEndpoint: " https://issuer.example/token ", RegistrationEndpoint: " https://issuer.example/register ", } - meta, err := resolveAuthorizationServer(context.Background(), http.DefaultClient, "http://0.0.0.0:0/invalid", cfg) + meta, err := resolveAuthorizationServer(context.Background(), client, "https://issuer.example", cfg) if err != nil { t.Fatalf("resolveAuthorizationServer() error = %v", err) }