Improve webclient non-200 error reporting (#67579)

* Improve webclient non-200 error reporting * Add comments to clarify JSON parsing behavior for 200 responses * Improve error message for unreadable response body * Fix error message assertion * Add debug logging for missing error message in JSON response * Adjust error message formatting and content
2026-08-31 01:50:50 +08:00 · 2026-06-17 13:23:47 -04:00
parent 263a1736cc
commit 673aab5042
2 changed files with 250 additions and 115 deletions
@@ -22,7 +22,6 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -193,6 +192,82 @@ func Find(cfg *Config) (*PingResponse, error) {
 	return findWithClient(cfg, clt)
 }

+// maxErrorResponseBodyBytes bounds how much of a non-200 response body is read
+// so a misbehaving upstream cannot make the client buffer an arbitrarily large
+// body just to build an error message.
+const maxErrorResponseBodyBytes = 4096
+
+// errorFromUnsuccessfulResponse builds an actionable error describing why a
+// request returned an unsuccessful HTTP status. The HTTP status and the
+// response body are surfaced so the caller can tell whether the proxy,
+// an intermediary, or a wrong address produced the failure.
+func errorFromUnsuccessfulResponse(ctx context.Context, endpoint, proxyAddr string, resp *http.Response) error {
+	slog.DebugContext(ctx, "Received unsuccessful response", "endpoint", endpoint, "code", resp.StatusCode)
+
+	target := "https://" + proxyAddr
+	reqURL := target + endpoint
+
+	var helpMessage string
+	switch {
+	case resp.StatusCode == http.StatusTooManyRequests:
+		helpMessage = "the server is rate-limiting requests, try again shortly"
+	case resp.StatusCode >= 500:
+		helpMessage = "the proxy may be unhealthy or temporarily unavailable"
+	default:
+		// A 4xx (most commonly 404) usually means proxyAddr is not a Teleport
+		// proxy, or something other than the proxy answered.
+		helpMessage = fmt.Sprintf("is %q a Teleport proxy?", target)
+	}
+
+	// A non-200 response is not necessarily from the proxy, something in front
+	// of it (a load balancer, a tunnel like Cloudflare, etc.) can return its own
+	// error page, which is untrusted and may be arbitrarily large.
+	bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxErrorResponseBodyBytes))
+	if err != nil {
+		return trace.Wrap(err, "%s returned HTTP %d but the response body could not be fully read", reqURL, resp.StatusCode)
+	}
+
+	if contentType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")); contentType != "application/json" {
+		slog.DebugContext(ctx, "Response is not JSON", "url", reqURL, "content_type", contentType, "body", string(bodyBytes), "error", err)
+		return trace.Errorf("%s returned HTTP %d; %s", reqURL, resp.StatusCode, helpMessage)
+	}
+
+	errResp := &PingErrorResponse{}
+	if err := json.Unmarshal(bodyBytes, errResp); err != nil {
+		slog.DebugContext(ctx, "Could not parse response body", "url", reqURL, "body", string(bodyBytes), "error", err)
+		return trace.Errorf("%s returned an unparseable HTTP %d JSON response; %s%s", reqURL, resp.StatusCode, helpMessage, snippetSuffix(bodyBytes))
+	}
+
+	if errResp.Error.Message == "" {
+		slog.DebugContext(ctx, "Parsed JSON response did not contain an error message", "url", reqURL, "body", string(bodyBytes))
+		return trace.Errorf("%s returned an HTTP %d JSON response with no error message; %s", reqURL, resp.StatusCode, helpMessage)
+	}
+
+	// The message may come from an intermediary rather than the proxy, so route
+	// it through snippetSuffix like any other untrusted text.
+	return trace.Errorf("%s returned HTTP %d%s", reqURL, resp.StatusCode, snippetSuffix([]byte(errResp.Error.Message)))
+}
+
+// snippetSuffix returns a single-line, quoted excerpt of the given text
+// formatted as an appendable error suffix: `: "…"` with the leading delimiter
+// included, or "" for empty text or whitespace only so callers can concatenate
+// it unconditionally.
+func snippetSuffix(text []byte) string {
+	s := strings.Join(strings.Fields(string(text)), " ")
+	if s == "" {
+		return ""
+	}
+	// Long enough to be useful for diagnosis, short enough not to flood the terminal.
+	const maxLen = 256
+	if len(s) > maxLen {
+		s = s[:maxLen] + "…"
+	}
+	// The %q is load-bearing, not cosmetic. The text is untrusted input and the
+	// result is printed to a terminal, so control bytes must be escaped rather
+	// than emitted raw (do not replace it with plain concatenation).
+	return fmt.Sprintf(": %q", s)
+}
+
 func findWithClient(cfg *Config, clt *http.Client) (*PingResponse, error) {
 	ctx, span := cfg.TraceProvider.Tracer("webclient").Start(cfg.Context, "webclient/Find")
 	defer span.End()
@@ -222,9 +297,19 @@ func findWithClient(cfg *Config, clt *http.Client) (*PingResponse, error) {
 	}

 	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, errorFromUnsuccessfulResponse(req.Context(), endpoint.Path, cfg.ProxyAddr, resp)
+	}
+
+	// In case of a 200 response, findWithClient immediately attempts to parse
+	// the response as JSON. Attempting to check Content-Type before parsing
+	// the response would be a breaking change for Teleport deployments that
+	// sit in front of misbehaving proxies that mangle Content-Type for whatever
+	// reason. Consider introducing that change only in a major release.
 	pr := &PingResponse{}
 	if err := json.NewDecoder(resp.Body).Decode(pr); err != nil {
-		return nil, trace.Wrap(err)
+		return nil, trace.Wrap(err, "cannot parse server find response; is %q a Teleport proxy?", "https://"+cfg.ProxyAddr)
 	}

 	return pr, nil
@@ -278,42 +363,17 @@ func pingWithClient(cfg *Config, clt *http.Client) (*PingResponse, error) {
 	defer resp.Body.Close()

 	if resp.StatusCode != http.StatusOK {
-		slog.DebugContext(req.Context(), "Received unsuccessful ping response", "code", resp.StatusCode)
-
-		bodyBytes, err := io.ReadAll(resp.Body)
-		if err != nil {
-			return nil, trace.Wrap(err, "could not read ping response body; check the network connection")
-		}
-
-		helpQuestion := "is proxy reachable?"
-		if resp.StatusCode == http.StatusNotFound {
-			// More often than not, a 404 from /webapi/ping is going to indicate
-			// that cfg.ProxyAddr is not a Teleport server in the first place.
-			helpQuestion = fmt.Sprintf("is %q a Teleport server?", "https://"+cfg.ProxyAddr)
-		}
-
-		// A non-200 response is not necessarily from the proxy. Something in front
-		// of it (a load balancer, a tunnel like Cloudflare, etc.) can return its
-		// own non-JSON error page.
-		// Only attempt to parse the body as JSON when the Content-Type says so.
-		if contentType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")); contentType != "application/json" {
-			slog.DebugContext(req.Context(), "Ping response is not JSON", "content_type", contentType, "body", string(bodyBytes), "error", err)
-
-			return nil, trace.Errorf("/webapi/ping returned a %d response; %s", resp.StatusCode, helpQuestion)
-		}
-
-		errResp := &PingErrorResponse{}
-		if err := json.Unmarshal(bodyBytes, errResp); err != nil {
-			slog.DebugContext(req.Context(), "Could not parse ping response body", "body", string(bodyBytes), "error", err)
-			return nil, trace.Errorf("/webapi/ping returned a %d JSON response; %s", resp.StatusCode, helpQuestion)
-		}
-
-		return nil, trace.Wrap(errors.New(errResp.Error.Message), "proxy service returned unsuccessful ping response; Teleport cluster auth may be misconfigured")
+		return nil, errorFromUnsuccessfulResponse(req.Context(), endpoint.Path, cfg.ProxyAddr, resp)
 	}

+	// In case of a 200 response, pingWithClient immediately attempts to parse
+	// the response as JSON. Attempting to check Content-Type before parsing
+	// the response would be a breaking change for Teleport deployments that
+	// sit in front of misbehaving proxies that mangle Content-Type for whatever
+	// reason. Consider introducing that change only in a major release.
 	pr := &PingResponse{}
 	if err := json.NewDecoder(resp.Body).Decode(pr); err != nil {
-		return nil, trace.Wrap(err, "cannot parse server ping response; is %q a Teleport server?", "https://"+cfg.ProxyAddr)
+		return nil, trace.Wrap(err, "cannot parse server ping response; is %q a Teleport proxy?", "https://"+cfg.ProxyAddr)
 	}

 	return pr, nil
@@ -19,12 +19,14 @@ package webclient
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net"
 	"net/http"
 	"net/http/httptest"
 	"slices"
 	"strings"
 	"testing"
+	"testing/iotest"
 	"time"

 	"github.com/google/go-cmp/cmp"
@@ -110,112 +112,185 @@ func TestPlainHttpFallback(t *testing.T) {
 	}
 }

-func TestPingError(t *testing.T) {
+func TestErrorFromUnsuccessfulResponse(t *testing.T) {
 	t.Parallel()

-	testCases := []struct {
+	const (
+		endpoint  = "/webapi/ping"
+		proxyAddr = "proxy.example.com:443"
+	)
+
+	cases := []struct {
 		desc        string
 		statusCode  int
 		contentType string
-		writeBody   func(t *testing.T, w http.ResponseWriter)
-		errContains string
+		body        string
+		bodyReadErr bool
+		errContains []string
+		errExcludes []string
 	}{
 		{
-			desc:        "unsuccessful response",
+			desc:        "structured error message",
 			statusCode:  http.StatusInternalServerError,
 			contentType: "application/json",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				err := json.NewEncoder(w).Encode(PingErrorResponse{Error: PingError{Message: "lorem ipsum"}})
-				require.NoError(t, err)
-			},
-			errContains: "lorem ipsum",
+			body:        `{"error":{"message":"something went wrong"}}`,
+			errContains: []string{"something went wrong", "returned HTTP 500"},
 		},
 		{
-			desc:        "mangled response",
+			desc:        "control sequences in server message are escaped",
+			statusCode:  http.StatusBadGateway,
+			contentType: "application/json",
+			body:        "{\"error\":{\"message\":\"evil\\u001b[2Jspoof\"}}",
+			errContains: []string{"returned HTTP 502", `evil\x1b[2Jspoof`},
+			errExcludes: []string{"\x1b"},
+		},
+		{
+			desc:        "long server message is truncated",
 			statusCode:  http.StatusInternalServerError,
 			contentType: "application/json",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				_, err := w.Write([]byte("mangled lorem ipsum"))
-				require.NoError(t, err)
-			},
-			errContains: "/webapi/ping returned a 500 JSON response; is proxy reachable?",
+			body:        `{"error":{"message":"` + strings.Repeat("a", 300) + `"}}`,
+			errContains: []string{"returned HTTP 500", strings.Repeat("a", 256) + "…"},
+			errExcludes: []string{strings.Repeat("a", 257)},
+		},
+		{
+			desc:        "structured error without message",
+			statusCode:  http.StatusInternalServerError,
+			contentType: "application/json",
+			body:        `{"error":{"message":""}}`,
+			errContains: []string{"HTTP 500 JSON response with no error message", "the proxy may be unhealthy"},
+		},
+		{
+			desc:        "unparseable JSON body",
+			statusCode:  http.StatusInternalServerError,
+			contentType: "application/json",
+			body:        "mangled",
+			errContains: []string{"unparseable HTTP 500 JSON response", `"mangled"`, "the proxy may be unhealthy"},
+		},
+		{
+			desc:        "oversized JSON body is bounded and unparseable",
+			statusCode:  http.StatusInternalServerError,
+			contentType: "application/json",
+			body:        `{"error":{"message":"` + strings.Repeat("a", maxErrorResponseBodyBytes) + `"}}`,
+			errContains: []string{"unparseable HTTP 500 JSON response"},
+			errExcludes: []string{strings.Repeat("a", maxErrorResponseBodyBytes)},
 		},
 		{
-			// Something in front of the proxy responded with its own non-JSON error
-			// page.
 			desc:        "non-JSON content type",
+			statusCode:  http.StatusBadGateway,
+			contentType: "text/html; charset=utf-8",
+			body:        "<html><body>error 502 from load balancer</body></html>",
+			errContains: []string{"returned HTTP 502", "the proxy may be unhealthy"},
+		},
+		{
+			desc:        "missing content type",
+			statusCode:  http.StatusBadGateway,
+			contentType: "",
+			body:        "bad gateway",
+			errContains: []string{"returned HTTP 502", "the proxy may be unhealthy"},
+		},
+		{
+			desc:        "non-JSON 404",
+			statusCode:  http.StatusNotFound,
+			contentType: "text/html; charset=utf-8",
+			body:        "<html><body>not found</body></html>",
+			errContains: []string{"returned HTTP 404", `is "https://` + proxyAddr + `" a Teleport proxy?`},
+		},
+		{
+			desc:        "unparseable JSON 404",
+			statusCode:  http.StatusNotFound,
+			contentType: "application/json",
+			body:        "mangled",
+			errContains: []string{"unparseable HTTP 404 JSON response", `a Teleport proxy?`},
+		},
+		{
+			desc:        "rate limited",
+			statusCode:  http.StatusTooManyRequests,
+			contentType: "text/html; charset=utf-8",
+			body:        "slow down",
+			errContains: []string{"returned HTTP 429", "rate-limiting"},
+		},
+		{
+			desc:        "body read error",
 			statusCode:  http.StatusInternalServerError,
-			contentType: "text/html; charset=utf-8",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				_, err := w.Write([]byte("<html><body>error 502</body></html>"))
-				require.NoError(t, err)
-			},
-			errContains: "/webapi/ping returned a 500 response; is proxy reachable?",
-		},
-		{
-			// A 404 on /webapi/ping suggests the address isn't a Teleport server at
-			// all rather than the proxy being unreachable.
-			desc:        "non-JSON 404 response",
-			statusCode:  http.StatusNotFound,
-			contentType: "text/html; charset=utf-8",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				_, err := w.Write([]byte("<html><body>not found</body></html>"))
-				require.NoError(t, err)
-			},
-			errContains: `/webapi/ping returned a 404 response; is "https://`,
-		},
-		{
-			// 404 + application/json but the body isn't parseable. The help text
-			// should still reflect that the address probably isn't a Teleport server.
-			desc:        "mangled 404 response",
-			statusCode:  http.StatusNotFound,
 			contentType: "application/json",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				_, err := w.Write([]byte("mangled lorem ipsum"))
-				require.NoError(t, err)
-			},
-			errContains: `/webapi/ping returned a 404 JSON response; is "https://`,
-		},
-		{
-			// 200 but a body that doesn't decode into PingResponse.
-			// In theory, we could check Content-Type on success too, but what if
-			// there's a deployment behind something that mangles Content-Type?
-			// Checking Content-Type on success would introduce a regression and make
-			// it impossible for users to log in.
-			desc:        "mangled 200 response",
-			statusCode:  http.StatusOK,
-			contentType: "application/json",
-			writeBody: func(t *testing.T, w http.ResponseWriter) {
-				_, err := w.Write([]byte("mangled lorem ipsum"))
-				require.NoError(t, err)
-			},
-			errContains: `cannot parse server ping response; is "https://`,
+			bodyReadErr: true,
+			errContains: []string{"HTTP 500", "could not be fully read"},
 		},
 	}

-	for _, testCase := range testCases {
-		t.Run(testCase.desc, func(t *testing.T) {
-			handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-				if req.RequestURI != "/webapi/ping" {
-					w.WriteHeader(http.StatusNotFound)
-					return
-				}
-
-				w.Header().Set("Content-Type", testCase.contentType)
-				w.WriteHeader(testCase.statusCode)
-				testCase.writeBody(t, w)
-			})
-			httpSvr := httptest.NewServer(handler)
-			defer httpSvr.Close()
-			proxyAddr := httpSvr.Listener.Addr().String()
-
-			_, err := Ping(
-				&Config{Context: context.Background(), ProxyAddr: proxyAddr, Insecure: true})
-			require.ErrorContains(t, err, testCase.errContains)
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			header := http.Header{}
+			if tc.contentType != "" {
+				header.Set("Content-Type", tc.contentType)
+			}
+			var body io.Reader = strings.NewReader(tc.body)
+			if tc.bodyReadErr {
+				body = iotest.ErrReader(io.ErrUnexpectedEOF)
+			}
+			resp := &http.Response{
+				StatusCode: tc.statusCode,
+				Header:     header,
+				Body:       io.NopCloser(body),
+			}
+			err := errorFromUnsuccessfulResponse(context.Background(), endpoint, proxyAddr, resp)
+			for _, want := range tc.errContains {
+				require.ErrorContains(t, err, want)
+			}
+			for _, unwanted := range tc.errExcludes {
+				require.NotContains(t, err.Error(), unwanted)
+			}
 		})
 	}
 }

+func TestPingUnsuccessfulResponse(t *testing.T) {
+	t.Parallel()
+
+	t.Run("routes non-200 to helper", func(t *testing.T) {
+		proxyAddr := startProxy(t, "/webapi/ping", http.StatusInternalServerError, "application/json", "mangled")
+		_, err := Ping(&Config{Context: context.Background(), ProxyAddr: proxyAddr, Insecure: true})
+		require.ErrorContains(t, err, "/webapi/ping returned an unparseable HTTP 500 JSON response")
+	})
+
+	t.Run("unparseable 200 response", func(t *testing.T) {
+		proxyAddr := startProxy(t, "/webapi/ping", http.StatusOK, "application/json", "mangled")
+		_, err := Ping(&Config{Context: context.Background(), ProxyAddr: proxyAddr, Insecure: true})
+		require.ErrorContains(t, err, "cannot parse server ping response")
+	})
+}
+
+func TestFindUnsuccessfulResponse(t *testing.T) {
+	t.Parallel()
+
+	t.Run("routes non-200 to helper", func(t *testing.T) {
+		proxyAddr := startProxy(t, "/webapi/find", http.StatusInternalServerError, "application/json", "mangled")
+		_, err := Find(&Config{Context: context.Background(), ProxyAddr: proxyAddr, Insecure: true})
+		require.ErrorContains(t, err, "/webapi/find returned an unparseable HTTP 500 JSON response")
+	})
+
+	t.Run("unparseable 200 response", func(t *testing.T) {
+		proxyAddr := startProxy(t, "/webapi/find", http.StatusOK, "application/json", "mangled")
+		_, err := Find(&Config{Context: context.Background(), ProxyAddr: proxyAddr, Insecure: true})
+		require.ErrorContains(t, err, "cannot parse server find response")
+	})
+}
+
+func startProxy(t *testing.T, wantPath string, status int, contentType, body string) string {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != wantPath {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		w.Header().Set("Content-Type", contentType)
+		w.WriteHeader(status)
+		_, _ = io.WriteString(w, body)
+	}))
+	t.Cleanup(srv.Close)
+	return srv.Listener.Addr().String()
+}
+
 func TestTunnelAddr(t *testing.T) {
 	cases := []struct {
 		name               string