diff --git a/cmd/mcp-http/main.go b/cmd/mcp-http/main.go index a9b559c0..099b8ee7 100644 --- a/cmd/mcp-http/main.go +++ b/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()), ) diff --git a/internal/auth/bearer_info.go b/internal/auth/bearer_info.go index a58f4433..ebdc550e 100644 --- a/internal/auth/bearer_info.go +++ b/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) + } +} diff --git a/internal/auth/bearer_info_test.go b/internal/auth/bearer_info_test.go new file mode 100644 index 00000000..be1b77ea --- /dev/null +++ b/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) + } + } +}