From 14fcb6469db5ec64858de142dce4c744dc10d67b Mon Sep 17 00:00:00 2001 From: AdaAibaby <8872412+adababys@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:35:28 +0800 Subject: [PATCH] fix(api): classify client-disconnect body-read errors separately from auth failures kin-openapi reads the full request body (io.ReadAll) before calling any auth function. When that read fails - the client dropped the connection, or the server read deadline fired while the goroutine was queued under high concurrency - kin-openapi returns a RequestError{Reason:"reading failed"} wrapping a net.Error, and that is carried inside the SecurityRequirementsError because the body is read before auth runs. On current main such a network failure flows through ProcessSecurityErrors and is reported as a SecurityRequirementsError: logged WARN, counted as a 400/401 auth error in metrics, and marked as an error on the OTel span. All three are misleading - nothing was wrong with the caller credentials; the connection just went away. Detect this case in MultiErrorHandler (new clientDisconnectReason helper): a RequestError{reading failed} whose cause is a net.Error is tagged with clientDisconnectPrefix instead of going to ProcessSecurityErrors. ErrorHandler then records it as an informational span event (not an error) and returns 499 Client Closed Request, skipping c.Errors so it never looks like an auth failure. A reading failure with a non-network cause, and genuine auth errors, are untouched. Rebased onto current main at the maintainers invitation on #3414, adapted to the refactored error helper (processCustomErrors -> sharedauth. ProcessSecurityErrors). Adds tests for the disconnect case as requested: classification in MultiErrorHandler, the 499/no-c.Errors behaviour in ErrorHandler, and an end-to-end pass through both stages. Closes #3413. Supersedes #3414. --- packages/api/internal/utils/error.go | 57 +++++++++++ packages/api/internal/utils/error_test.go | 112 ++++++++++++++++++++++ 2 files changed, 169 insertions(+) diff --git a/packages/api/internal/utils/error.go b/packages/api/internal/utils/error.go index e01256e76c..9e79e19085 100644 --- a/packages/api/internal/utils/error.go +++ b/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,6 +185,15 @@ 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, @@ -164,3 +201,23 @@ func MultiErrorHandler(me openapi3.MultiError) 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 +} diff --git a/packages/api/internal/utils/error_test.go b/packages/api/internal/utils/error_test.go index f10722c5bb..7b68c56c77 100644 --- a/packages/api/internal/utils/error_test.go +++ b/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) + }) +}