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) + }) +}