fix(upstream): stop misclassifying transport errors as auth failures (#464)

The auth-strategy loop in connection_http.go used `isAuthError` to decide
whether to fall back to the next strategy. The substring list was too
permissive — `"auth"` matched the strategy-name wrapper itself
("...headers-auth strategy: ...", "...no-auth strategy: ..."), so any
wrapped transport/parse error was treated as an auth failure and silently
triggered the OAuth fallback.

The visible symptom: a server configured with a static Bearer token whose
upstream returns HTTP 502 (Cloudflare error body, not JSON) failed with
"OAuth authentication required" — pointing the user at the wrong fix.

Tighten the substring list to genuine 403 / "authentication failed" /
"authorization failed" markers. 401/Unauthorized continues to be routed
through isOAuthError, unchanged.

Co-authored-by: Claude Code <noreply@anthropic.com>

Author: Dumbris

internal/upstream/core/auth_classification_test.go

@@ -0,0 +1,62 @@
+package core
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestIsAuthError_DoesNotMatchStrategyWrapper verifies that the substring "auth"
+// inside the strategy-name wrapper ("headers-auth strategy", "no-auth strategy")
+// does NOT cause a non-auth transport/parse error to be misclassified as an
+// authentication failure. Misclassification triggered an unintended OAuth
+// fallback and surfaced a misleading "OAuth authentication required" message
+// to users whose static Bearer token was actually fine (e.g. when the upstream
+// returned HTTP 502 with a non-JSON body).
+func TestIsAuthError_DoesNotMatchStrategyWrapper(t *testing.T) {
+	c := &Client{}
+
+	// Real scenario: upstream returns 502 with non-JSON body; initialize parse
+	// fails; tryHeadersAuth wraps with "headers-auth strategy".
+	upstream502 := errors.New("MCP initialize failed during headers-auth strategy: failed to unmarshal response: unexpected end of JSON input")
+	assert.False(t, c.isAuthError(upstream502),
+		"a JSON-parse failure wrapped by the headers-auth strategy must not be classified as an auth error")
+
+	noAuthWrapper := errors.New("MCP initialize failed during no-auth strategy: failed to unmarshal response: unexpected end of JSON input")
+	assert.False(t, c.isAuthError(noAuthWrapper),
+		"a JSON-parse failure wrapped by the no-auth strategy must not be classified as an auth error")
+
+	sseWrapper := errors.New("MCP initialize failed during SSE headers-auth strategy: context deadline exceeded")
+	assert.False(t, c.isAuthError(sseWrapper),
+		"a timeout wrapped by the SSE headers-auth strategy must not be classified as an auth error")
+}
+
+// TestIsAuthError_MatchesGenuineAuthFailures verifies that real 403 / explicit
+// authentication-failure responses still trigger the fallback chain.
+// Note: 401/Unauthorized is intentionally classified as an OAuth error (see
+// isOAuthError), so it is exercised via TestIsAuthError_IgnoresOAuthErrors.
+func TestIsAuthError_MatchesGenuineAuthFailures(t *testing.T) {
+	c := &Client{}
+
+	cases := []string{
+		"server returned status 403 Forbidden",
+		"403: forbidden",
+		"authentication failed for user",
+		"authorization failed",
+	}
+	for _, msg := range cases {
+		assert.True(t, c.isAuthError(errors.New(msg)),
+			"expected isAuthError to match genuine auth failure: %q", msg)
+	}
+}
+
+// TestIsAuthError_IgnoresOAuthErrors ensures OAuth-classified errors are not
+// double-classified as generic auth errors (the OAuth branch handles them).
+func TestIsAuthError_IgnoresOAuthErrors(t *testing.T) {
+	c := &Client{}
+	err := errors.New("no valid token available, authorization required")
+	assert.True(t, c.isOAuthError(err), "sanity: should be an OAuth error")
+	assert.False(t, c.isAuthError(err),
+		"OAuth errors must not also be classified as generic auth errors")
+}

internal/upstream/core/connection_http.go

@@ -251,7 +251,16 @@ func (c *Client) trySSENoAuth(ctx context.Context) error {
 	return nil
 }
 
-// isAuthError checks if error indicates authentication failure (non-OAuth)
+// isAuthError checks if error indicates authentication failure (non-OAuth).
+//
+// The substring list is intentionally narrow: the strategy wrappers
+// ("MCP initialize failed during headers-auth strategy", "no-auth strategy",
+// "SSE headers-auth strategy") contain the literal token "auth", so any
+// substring as permissive as "auth" or "authentication" would misclassify a
+// wrapped transport/parse error (e.g. upstream HTTP 502 → JSON parse failure)
+// as an auth failure and trigger an unwanted OAuth fallback. The patterns
+// below match only genuine HTTP 403 responses and explicit "*failed"/"*failure"
+// phrasings that upstreams emit, never the wrapper text.
 func (c *Client) isAuthError(err error) bool {
 	if err == nil {
 		return false
@@ -264,8 +273,9 @@ func (c *Client) isAuthError(err error) bool {
 
 	errStr := err.Error()
 	return containsAny(errStr, []string{
-		"403", "Forbidden",
-		"authentication", "auth",
+		"403", "Forbidden", "forbidden",
+		"authentication failed", "authentication failure",
+		"authorization failed", "authorization failure",
 	})
 }