diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index d84018856a..6f9c13f880 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -429,6 +429,17 @@ func wrapBackendError(err error, backendID string, operation string) error { return fmt.Errorf("%w: failed to %s for backend %s: %v", vmcp.ErrAuthenticationFailed, operation, backendID, err) } + // transport.ErrAuthorizationRequired is returned (wrapped in *transport.Error + // and *transport.AuthorizationRequiredError) for 401 responses with a + // WWW-Authenticate header. transport.ErrOAuthAuthorizationRequired is the + // companion sentinel from the OAuth-handler path. Both must map to + // ErrAuthenticationFailed so health monitoring engages the auth-aware + // branch (#4935) instead of treating the probe as unhealthy (#5223). + if errors.Is(err, transport.ErrAuthorizationRequired) || + errors.Is(err, transport.ErrOAuthAuthorizationRequired) { + return fmt.Errorf("%w: failed to %s for backend %s: %v", + vmcp.ErrAuthenticationFailed, operation, backendID, err) + } // ErrLegacySSEServer is returned for any 4xx (except 401) on initialize POST. // This includes 403 (auth rejection) and 404/405 (endpoint not found/method not allowed). // We cannot distinguish auth failures from routing errors without the raw status code, diff --git a/pkg/vmcp/client/client_test.go b/pkg/vmcp/client/client_test.go index 182ccd6ce6..57ad5e0187 100644 --- a/pkg/vmcp/client/client_test.go +++ b/pkg/vmcp/client/client_test.go @@ -1001,6 +1001,34 @@ func TestWrapBackendError(t *testing.T) { err: context.DeadlineExceeded, wantSentinel: vmcp.ErrTimeout, }, + { + // mcp-go v0.49.0 added ErrAuthorizationRequired for 401 responses + // with a WWW-Authenticate header. Issue #5223: probe of GitHub + // Copilot's MCP returns this; without recognizing the sentinel here + // the error falls through to ErrBackendUnavailable and the health + // monitor never engages the auth-aware branch (#4935). + name: "ErrAuthorizationRequired maps to ErrAuthenticationFailed", + err: transport.ErrAuthorizationRequired, + wantSentinel: vmcp.ErrAuthenticationFailed, + }, + { + // Production chain mcp-go produces: transport.Error wrapping + // *AuthorizationRequiredError. Error() formats as + // "transport error: authorization required" — byte-for-byte + // the string seen in North's WARN logs (issue #5223). Both + // layers Unwrap() to the sentinel so errors.Is must succeed. + name: "wrapped AuthorizationRequiredError (production chain) maps to ErrAuthenticationFailed", + err: transport.NewError(&transport.AuthorizationRequiredError{}), + wantSentinel: vmcp.ErrAuthenticationFailed, + }, + { + // Companion sentinel for the OAuth-handler path. Less hot in + // practice but belongs in the same matcher block; the existing + // transport.ErrUnauthorized check would not catch it. + name: "ErrOAuthAuthorizationRequired maps to ErrAuthenticationFailed", + err: transport.ErrOAuthAuthorizationRequired, + wantSentinel: vmcp.ErrAuthenticationFailed, + }, } for _, tt := range tests { diff --git a/pkg/vmcp/errors.go b/pkg/vmcp/errors.go index f2b8e58868..1458f3a5ce 100644 --- a/pkg/vmcp/errors.go +++ b/pkg/vmcp/errors.go @@ -119,6 +119,14 @@ func IsAuthenticationError(err error) bool { return true } + // mcp-go ErrAuthorizationRequired ("authorization required") and + // ErrOAuthAuthorizationRequired ("no valid token available, authorization + // required") string forms. Used as a fallback when the typed sentinel is + // not preserved through the error chain (issue #5223). + if strings.Contains(errLower, "authorization required") { + return true + } + return false } diff --git a/pkg/vmcp/health/checker_test.go b/pkg/vmcp/health/checker_test.go index 113524285a..47d3881edc 100644 --- a/pkg/vmcp/health/checker_test.go +++ b/pkg/vmcp/health/checker_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/mark3labs/mcp-go/client/transport" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -450,6 +451,14 @@ func TestIsAuthenticationError(t *testing.T) { {name: "404 not found", err: errors.New("404 not found"), expectErr: false}, {name: "500 internal server error", err: errors.New("500 internal server error"), expectErr: false}, {name: "hostname with 401", err: errors.New("http://backend401.example.com"), expectErr: false}, + // Pin against accidental loosening of the "authorization required" + // substring matcher: a validation message of the form "field + // 'authorization' required" must not be misclassified as an auth + // failure. The current matcher uses the contiguous substring + // "authorization required" (one space, no punctuation), so this + // string does not match — but a future loosening (e.g. allowing + // any whitespace) would silently regress. + {name: "validation message containing 'authorization' and 'required'", err: errors.New("field 'authorization' required"), expectErr: false}, {name: "nil error", err: nil, expectErr: false}, } @@ -633,6 +642,16 @@ func TestHealthChecker_CheckHealth_AuthErrorsCategorizesAsUnauthenticated(t *tes name: "mcp-go ErrUnauthorized wrapped as ErrAuthenticationFailed by wrapBackendError", err: fmt.Errorf("%w: failed to initialize for backend my-backend: unauthorized (401)", vmcp.ErrAuthenticationFailed), }, + { + // Issue #5223: mcp-go v0.49.0+ returns *AuthorizationRequiredError for + // 401 with WWW-Authenticate, wrapped in *transport.Error. Both layers + // Unwrap() to transport.ErrAuthorizationRequired. The string fallback + // must recognize "authorization required" so the probe error reaches + // health monitoring as BackendUnauthenticated rather than falling + // through to BackendUnhealthy and producing WARN spam. + name: "transport.Error wrapping AuthorizationRequiredError", + err: transport.NewError(&transport.AuthorizationRequiredError{}), + }, } for _, tt := range tests { @@ -697,6 +716,31 @@ func TestHealthChecker_CheckHealth_AuthErrorWithOutgoingAuthIsHealthy(t *testing authConfig: &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeUpstreamInject}, err: fmt.Errorf("%w: unauthorized (401)", vmcp.ErrAuthenticationFailed), }, + { + // Issue #5223 exact reproducer: North's github-copilot-entry backend + // behind an upstreamInject auth strategy. Probe carries no user creds; + // mcp-go returns the typed authorization-required chain. Once correctly + // classified as auth, #4935's logic must take over and report + // BackendHealthy with nil err so the monitor records a successful + // check, the circuit breaker stays closed, and the WARN spam stops. + name: "upstream_inject + transport.Error wrapping AuthorizationRequiredError", + authConfig: &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeUpstreamInject}, + err: transport.NewError(&transport.AuthorizationRequiredError{}), + }, + { + // Same mcp-go chain as the upstream_inject row above; this row pins + // that token_exchange flows through the same authErrorStatus branch. + name: "token_exchange + transport.Error wrapping AuthorizationRequiredError", + authConfig: &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeTokenExchange}, + err: transport.NewError(&transport.AuthorizationRequiredError{}), + }, + { + // Same mcp-go chain as the upstream_inject row above; this row pins + // that header_injection flows through the same authErrorStatus branch. + name: "header_injection + transport.Error wrapping AuthorizationRequiredError", + authConfig: &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeHeaderInjection}, + err: transport.NewError(&transport.AuthorizationRequiredError{}), + }, } for _, tt := range tests {