fix(api): classify client-disconnect body-read errors separately from auth failures (rebase of #3414)

packages/api/internal/utils/error.go

@@ -3,6 +3,7 @@ package utils
 import (
 	"errors"
 	"fmt"
+	"net"
 	"net/http"
 	"strings"
 
@@ -21,6 +22,17 @@ import (
 // selector or the query, so it is dropped rather than reported.
 var errSecretsRejected = errors.New("secrets request rejected before the handler")
 
+const (
+	// clientDisconnectPrefix tags a validation failure that is really a failed
+	// request-body read caused by a network error, so ErrorHandler can treat it
+	// as a client disconnect instead of an authentication failure.
+	clientDisconnectPrefix = "client disconnected: "
+
+	// clientClosedRequestStatus is the non-standard 499 "Client Closed Request":
+	// the client went away before the server produced a response.
+	clientClosedRequestStatus = 499
+)
+
 // secretsErrorMessage is the fixed text such a request gets back.
 func secretsErrorMessage(statusCode int) string {
 	switch statusCode {
@@ -40,6 +52,22 @@ func ErrorHandler(c *gin.Context, message string, statusCode int) {
 
 	ctx := c.Request.Context()
 
+	// Client dropped the connection (or the server read deadline fired) before
+	// the request body was fully received. kin-openapi reads the body before
+	// auth, so a failed read is wrapped as a SecurityRequirementsError and
+	// MultiErrorHandler tags it with clientDisconnectPrefix. This is a
+	// client-side network event, not a server or auth error: record it
+	// informationally and return 499 so logs and metrics keep it separate from
+	// real authentication failures.
+	if after, ok := strings.CutPrefix(message, clientDisconnectPrefix); ok {
+		telemetry.ReportEvent(ctx, "client disconnected before request body received",
+			attribute.String("disconnect.reason", after),
+		)
+		c.AbortWithStatus(clientClosedRequestStatus)
+
+		return
+	}
+
 	// Secret management routes answer with fixed text and report a fixed
 	// error: neither the raw body (which GetRawData would read below) nor the
 	// upstream message may reach the client, the access log or the span.
@@ -157,10 +185,39 @@ func MultiErrorHandler(me openapi3.MultiError) error {
 
 		return fmt.Errorf("error in openapi3filter.RequestError: %s", errorLines[0])
 	case *openapi3filter.SecurityRequirementsError:
+		// A failed request-body read (client timeout / dropped connection) is
+		// returned by kin-openapi as a RequestError{Reason:"reading failed"}
+		// wrapped in the SecurityRequirementsError, because the body is read
+		// before auth runs. Classify it as a client disconnect so it is not
+		// reported as an authentication failure.
+		if reason, ok := clientDisconnectReason(e); ok {
+			return fmt.Errorf("%s%s", clientDisconnectPrefix, reason)
+		}
+
 		return sharedauth.ProcessSecurityErrors(e)
 	default:
 		// This should never happen today, but if our upstream code changes,
 		// we don't want to crash the server, so handle the unexpected error.
 		return fmt.Errorf("error validating request: %w", err)
 	}
 }
+
+// clientDisconnectReason reports whether a SecurityRequirementsError is really a
+// failed request-body read caused by a network error (the client went away or
+// the server read deadline fired) rather than a genuine auth rejection, and
+// returns the underlying network error text when so.
+func clientDisconnectReason(e *openapi3filter.SecurityRequirementsError) (string, bool) {
+	for _, err := range e.Errors {
+		var reqErr *openapi3filter.RequestError
+		if !errors.As(err, &reqErr) || reqErr.Reason != "reading failed" {
+			continue
+		}
+
+		var netErr net.Error
+		if errors.As(reqErr.Err, &netErr) {
+			return reqErr.Err.Error(), true
+		}
+	}
+
+	return "", false
+}

packages/api/internal/utils/error_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -302,3 +303,114 @@ func TestErrorHandlerStillRecordsTheBodyElsewhere(t *testing.T) {
 	require.NotEmpty(t, c.Errors)
 	assert.Contains(t, c.Errors[0].Error(), marker)
 }
+
+// fakeNetError is a net.Error used to simulate a body-read network failure
+// (client disconnect / server read deadline) the way kin-openapi surfaces it.
+type fakeNetError struct{ timeout bool }
+
+func (e fakeNetError) Error() string   { return "read tcp: i/o timeout" }
+func (e fakeNetError) Timeout() bool   { return e.timeout }
+func (e fakeNetError) Temporary() bool { return false }
+
+var _ net.Error = fakeNetError{}
+
+// readingFailed builds the SecurityRequirementsError kin-openapi returns when
+// the request body read fails before auth runs: a RequestError{reading failed}
+// wrapping cause, carried among the security errors.
+func readingFailed(cause error) *openapi3filter.SecurityRequirementsError {
+	return &openapi3filter.SecurityRequirementsError{
+		Errors: []error{
+			&openapi3filter.RequestError{Reason: "reading failed", Err: cause},
+		},
+	}
+}
+
+// TestMultiErrorHandlerClassifiesClientDisconnect proves a failed body read
+// caused by a network error is tagged as a client disconnect, while a reading
+// failure with a non-network cause, and a genuine auth error, are not.
+func TestMultiErrorHandlerClassifiesClientDisconnect(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		err            *openapi3filter.SecurityRequirementsError
+		wantDisconnect bool
+	}{
+		"body read timed out on the network": {
+			err:            readingFailed(fakeNetError{timeout: true}),
+			wantDisconnect: true,
+		},
+		"body read failed for a non-network reason stays an auth error": {
+			err:            readingFailed(errors.New("malformed chunked encoding")),
+			wantDisconnect: false,
+		},
+		"a real missing-credential error stays an auth error": {
+			err:            &openapi3filter.SecurityRequirementsError{Errors: []error{fmt.Errorf("Invalid API key. %w", sharedauth.ErrNoAuthHeader)}},
+			wantDisconnect: false,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := MultiErrorHandler(openapi3.MultiError{tc.err}).Error()
+			if tc.wantDisconnect {
+				assert.True(t, strings.HasPrefix(got, "client disconnected: "),
+					"want client-disconnect prefix, got %q", got)
+			} else {
+				assert.False(t, strings.HasPrefix(got, "client disconnected: "),
+					"did not want client-disconnect prefix, got %q", got)
+			}
+		})
+	}
+}
+
+// TestErrorHandlerClientDisconnectReturns499 proves a client-disconnect message
+// is answered with 499, carries no response body, and is not pushed onto
+// c.Errors (it is an informational span event, not a reported error).
+func TestErrorHandlerClientDisconnectReturns499(t *testing.T) {
+	t.Parallel()
+
+	recorder := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(recorder)
+	c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/sandboxes", nil)
+
+	ErrorHandler(c, "client disconnected: read tcp: i/o timeout", http.StatusUnauthorized)
+
+	assert.Equal(t, 499, recorder.Code)
+	assert.Empty(t, recorder.Body.Bytes(), "a disconnected client should get no response body")
+	assert.Empty(t, c.Errors, "a client disconnect must not be recorded as an error")
+}
+
+// TestMultiToErrorHandlerClientDisconnectEndToEnd runs the real validator output
+// through both stages: a network body-read failure becomes a 499, while the same
+// shape with a non-network cause remains an auth 401.
+func TestMultiToErrorHandlerClientDisconnectEndToEnd(t *testing.T) {
+	t.Parallel()
+
+	t.Run("network read failure -> 499", func(t *testing.T) {
+		t.Parallel()
+		msg := MultiErrorHandler(openapi3.MultiError{readingFailed(fakeNetError{timeout: true})}).Error()
+
+		recorder := httptest.NewRecorder()
+		c, _ := gin.CreateTestContext(recorder)
+		c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/sandboxes", nil)
+		ErrorHandler(c, msg, http.StatusUnauthorized)
+
+		assert.Equal(t, 499, recorder.Code)
+		assert.Empty(t, c.Errors)
+	})
+
+	t.Run("non-network read failure -> auth error", func(t *testing.T) {
+		t.Parallel()
+		msg := MultiErrorHandler(openapi3.MultiError{readingFailed(errors.New("malformed chunked encoding"))}).Error()
+
+		recorder := httptest.NewRecorder()
+		c, _ := gin.CreateTestContext(recorder)
+		c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/sandboxes", nil)
+		ErrorHandler(c, msg, http.StatusUnauthorized)
+
+		assert.NotEqual(t, 499, recorder.Code)
+		assert.NotEmpty(t, c.Errors)
+	})
+}