diff --git a/internal/mcp/oauth.go b/internal/mcp/oauth.go index 9106fdf5b..b435fbd7f 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: strings.TrimSpace(cfg.AuthorizationEndpoint), + TokenEndpoint: strings.TrimSpace(cfg.TokenEndpoint), + RegistrationEndpoint: strings.TrimSpace(cfg.RegistrationEndpoint), + } + return metadata, nil + } + discoveryBase := strings.TrimSpace(cfg.IssuerURL) if discoveryBase == "" { discoveryBase = baseURL diff --git a/internal/mcp/oauth_test.go b/internal/mcp/oauth_test.go index 610d6da44..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" @@ -51,7 +52,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 +63,47 @@ 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) + } +} + +// 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. + // 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(), client, "https://issuer.example", 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 != cfg.TokenEndpoint { - t.Fatalf("token endpoint = %q, want config fallback", meta.TokenEndpoint) + if meta.TokenEndpoint != "https://issuer.example/token" { + t.Fatalf("token endpoint = %q, want trimmed config value", 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)