Fix: Don't tell clients to re-authorise when we failed to validate

cmd/mcp-http/main.go

@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -476,13 +477,37 @@ func authMiddleware(resources config.Resources, next http.Handler) http.Handler
 		bearerToken := matches[1]
 
 		info, err := auth.GetBearerInfo(r.Context(), resources, bearerToken)
-		if err == auth.ErrBearerInfoUnauthorized {
+		switch {
+		case errors.Is(err, auth.ErrBearerInfoUnauthorized):
+			// The token was positively rejected, so challenge the client to
+			// re-authorise.
 			// https://datatracker.ietf.org/doc/html/rfc9728#name-www-authenticate-response
 			w.Header().Set("WWW-Authenticate",
 				`Bearer resource_metadata="`+resources.Info.MCPURL+`/.well-known/oauth-protected-resource"`)
 			http.Error(w, "Unauthorized", http.StatusUnauthorized)
 			return
-		} else if err != nil {
+
+		case errors.Is(err, auth.ErrBearerInfoCanceled):
+			// The client hung up while we were validating. Nobody is left to
+			// read a response and nothing failed on our side, so this is debug
+			// noise rather than an error.
+			requestLogger.DebugContext(r.Context(), "bearer info validation canceled by client",
+				slog.String("error", err.Error()),
+			)
+			return
+
+		case errors.Is(err, auth.ErrBearerInfoUnavailable):
+			// We could not determine whether the token is valid. Never send a
+			// re-authorisation challenge here: the token is probably fine, and
+			// telling the client to discard it puts the user through the whole
+			// OAuth flow (and MFA) over what may be a momentary blip.
+			requestLogger.ErrorContext(r.Context(), "failed to validate bearer info",
+				slog.String("error", err.Error()),
+			)
+			http.Error(w, "Failed to validate bearer token", http.StatusServiceUnavailable)
+			return
+
+		case err != nil:
 			requestLogger.ErrorContext(r.Context(), "failed to get bearer info",
 				slog.String("error", err.Error()),
 			)

internal/auth/bearer_info.go

@@ -11,10 +11,24 @@ import (
 	"github.com/teamwork/mcp/internal/config"
 )
 
-// ErrBearerInfoUnauthorized is returned when the bearer token is invalid or
-// unauthorized.
+// ErrBearerInfoUnauthorized is returned when Teamwork API positively rejected
+// the bearer token. This is the only error that may be turned into a 401 for
+// the MCP client: a 401 carries an RFC 9728 re-authorisation challenge, which
+// makes the client discard the token and run the OAuth flow again.
 var ErrBearerInfoUnauthorized = errors.New("unauthorized: failed to get bearer info")
 
+// ErrBearerInfoUnavailable is returned when the token could not be validated at
+// all — Teamwork API was unreachable, timed out, or answered with a server
+// error. The token may well be valid, so callers must not tell the client to
+// throw it away; report this as 503 instead.
+var ErrBearerInfoUnavailable = errors.New("unavailable: failed to validate bearer info")
+
+// ErrBearerInfoCanceled is returned when the caller's own request context was
+// cancelled while validating, which normally means the MCP client hung up
+// first. Nothing went wrong on our side and no response will be read, so this
+// is not worth reporting as a server error.
+var ErrBearerInfoCanceled = errors.New("canceled: bearer info validation abandoned")
+
 // BearerInfo contains information about the bearer token used to authenticate
 // with Teamwork API.
 type BearerInfo struct {
@@ -41,7 +55,13 @@ func GetBearerInfo(ctx context.Context, resources config.Resources, token string
 
 	response, err := resources.TeamworkHTTPClient().Do(authRequest)
 	if err != nil {
-		return nil, fmt.Errorf("failed to perform auth request: %w", err)
+		// Distinguish "the client went away" from "we could not reach Teamwork
+		// API": the former is routine (an MCP client closing its stream) and
+		// must not be logged as a server error, the latter is worth alerting on.
+		if ctx.Err() != nil || errors.Is(err, context.Canceled) {
+			return nil, fmt.Errorf("%w: %w", ErrBearerInfoCanceled, err)
+		}
+		return nil, fmt.Errorf("%w: failed to perform auth request: %w", ErrBearerInfoUnavailable, err)
 	}
 	defer func() {
 		if err := response.Body.Close(); err != nil {
@@ -51,15 +71,37 @@ func GetBearerInfo(ctx context.Context, resources config.Resources, token string
 		}
 	}()
 
-	if response.StatusCode != http.StatusOK {
-		return nil, ErrBearerInfoUnauthorized
+	if err := classifyAuthStatus(response.StatusCode, url); err != nil {
+		return nil, err
 	}
 
 	var info BearerInfo
 
 	decoder := json.NewDecoder(response.Body)
 	if err := decoder.Decode(&info); err != nil {
-		return nil, fmt.Errorf("failed to decode auth response: %w", err)
+		if ctx.Err() != nil || errors.Is(err, context.Canceled) {
+			return nil, fmt.Errorf("%w: %w", ErrBearerInfoCanceled, err)
+		}
+		return nil, fmt.Errorf("%w: failed to decode auth response: %w", ErrBearerInfoUnavailable, err)
 	}
 	return &info, nil
 }
+
+// classifyAuthStatus turns the status code of a userinfo response into an
+// error, or nil when the token was validated.
+//
+// Only 401 and 403 mean the token itself was refused. Every other non-200 —
+// 5xx, 429, a proxy error page — means validation never happened, and must stay
+// distinguishable: reporting those as a rejection is what let a momentary
+// upstream failure look to the OAuth client like an expired token, costing the
+// user a full re-authorisation.
+func classifyAuthStatus(statusCode int, url string) error {
+	switch statusCode {
+	case http.StatusOK:
+		return nil
+	case http.StatusUnauthorized, http.StatusForbidden:
+		return ErrBearerInfoUnauthorized
+	default:
+		return fmt.Errorf("%w: unexpected status %d from %s", ErrBearerInfoUnavailable, statusCode, url)
+	}
+}

internal/auth/bearer_info_test.go

@@ -0,0 +1,75 @@
+package auth
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+)
+
+func TestClassifyAuthStatus(t *testing.T) {
+	const url = "https://www.teamwork.com/launchpad/v1/userinfo.json"
+
+	tests := []struct {
+		name       string
+		statusCode int
+		want       error
+	}{
+		{name: "ok validates the token", statusCode: http.StatusOK, want: nil},
+
+		// Teamwork API looked at the token and refused it. Only these may lead
+		// to a re-authorisation challenge.
+		{name: "unauthorized is a rejection", statusCode: http.StatusUnauthorized, want: ErrBearerInfoUnauthorized},
+		{name: "forbidden is a rejection", statusCode: http.StatusForbidden, want: ErrBearerInfoUnauthorized},
+
+		// Validation never happened. These used to be reported as rejections,
+		// which made the client discard a valid token.
+		{name: "internal server error is inconclusive", statusCode: http.StatusInternalServerError,
+			want: ErrBearerInfoUnavailable},
+		{name: "bad gateway is inconclusive", statusCode: http.StatusBadGateway,
+			want: ErrBearerInfoUnavailable},
+		{name: "service unavailable is inconclusive", statusCode: http.StatusServiceUnavailable,
+			want: ErrBearerInfoUnavailable},
+		{name: "gateway timeout is inconclusive", statusCode: http.StatusGatewayTimeout,
+			want: ErrBearerInfoUnavailable},
+		{name: "too many requests is inconclusive", statusCode: http.StatusTooManyRequests,
+			want: ErrBearerInfoUnavailable},
+		{name: "bad request is inconclusive", statusCode: http.StatusBadRequest,
+			want: ErrBearerInfoUnavailable},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := classifyAuthStatus(tt.statusCode, url)
+			if tt.want == nil {
+				if err != nil {
+					t.Fatalf("classifyAuthStatus(%d) = %v, want nil", tt.statusCode, err)
+				}
+				return
+			}
+			if !errors.Is(err, tt.want) {
+				t.Fatalf("classifyAuthStatus(%d) = %v, want %v", tt.statusCode, err, tt.want)
+			}
+		})
+	}
+}
+
+// TestClassifyAuthStatusNeverConfusesUnavailableWithRejection guards the
+// invariant the middleware depends on: an inconclusive result must never be
+// mistaken for a rejection, because only a rejection is allowed to tell the
+// OAuth client to throw its token away.
+func TestClassifyAuthStatusNeverConfusesUnavailableWithRejection(t *testing.T) {
+	for statusCode := 100; statusCode < 600; statusCode++ {
+		err := classifyAuthStatus(statusCode, "https://example.com")
+		if err == nil {
+			continue
+		}
+		rejected := errors.Is(err, ErrBearerInfoUnauthorized)
+		if rejected != (statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden) {
+			t.Errorf("status %d: rejection = %v, want %v", statusCode, rejected,
+				statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden)
+		}
+		if rejected && errors.Is(err, ErrBearerInfoUnavailable) {
+			t.Errorf("status %d: classified as both a rejection and inconclusive", statusCode)
+		}
+	}
+}