From 9dcc893dec4bbcaf787f8b0541b53e5a91b37e48 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 11 Mar 2026 15:30:27 +0000 Subject: [PATCH 1/4] fix: WrapGRPCPublic preserves error code via sanitized TError detail WrapGRPCPublic was creating a bare gRPC status with no TError details, causing UnwrapGRPC on the client side to return ERR_ERROR (9) instead of the original error code. This broke errors.Is() detection for TX_EXISTS, TX_INVALID, and SERVICE_ERROR in gRPC clients like tx-blaster. Now attaches a single sanitized TError detail (code + message only). No file paths, line numbers, function names, wrapped chains, or error data are included, preserving the security properties. --- errors/errors.go | 13 ++++++++ errors/errors_test.go | 72 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index 961aec3993..57174c6b2f 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -726,6 +726,9 @@ func PublicError(err error) *Error { } // WrapGRPCPublic wraps an error for gRPC without exposing internal details. +// It includes a sanitized TError detail (code + message only) so that UnwrapGRPC +// can reconstruct the correct application error code on the client side. +// No file paths, line numbers, function names, wrapped error chains, or error data are included. func WrapGRPCPublic(err error) error { if err == nil { return nil @@ -737,6 +740,16 @@ func WrapGRPCPublic(err error) error { } st := status.New(ErrorCodeToGRPCCode(publicErr.code), publicErr.message) + + // Attach a sanitized TError detail so clients can recover the application error code + detail, pbErr := anypb.New(&TError{ + Code: publicErr.code, + Message: publicErr.message, + }) + if pbErr == nil { + st, _ = st.WithDetails(detail) + } + return st.Err() } diff --git a/errors/errors_test.go b/errors/errors_test.go index a721bcb51d..3ff4e64f0b 100644 --- a/errors/errors_test.go +++ b/errors/errors_test.go @@ -2215,7 +2215,7 @@ func TestWrapGRPCPublic(t *testing.T) { require.Nil(t, result) }) - t.Run("returns gRPC status error without internal details", func(t *testing.T) { + t.Run("returns gRPC status error with sanitized detail", func(t *testing.T) { innerErr := New(ERR_STORAGE_ERROR, "internal db error") outerErr := New(ERR_SERVICE_ERROR, "service unavailable", innerErr) @@ -2233,8 +2233,18 @@ func TestWrapGRPCPublic(t *testing.T) { // Message should be the top-level message only require.Equal(t, "service unavailable", st.Message()) - // Should not have detailed error information in status details - require.Empty(t, st.Details()) + // Should have exactly one sanitized TError detail (code + message only) + require.Len(t, st.Details(), 1) + + // Verify the detail contains only code and message, no internal info + unwrapped := UnwrapGRPC(result) + require.Equal(t, ERR_SERVICE_ERROR, unwrapped.code) + require.Equal(t, "service unavailable", unwrapped.message) + require.Empty(t, unwrapped.file) + require.Zero(t, unwrapped.line) + require.Empty(t, unwrapped.function) + require.Nil(t, unwrapped.wrappedErr) + require.Nil(t, unwrapped.data) }) t.Run("maps error codes to gRPC codes correctly", func(t *testing.T) { @@ -2297,6 +2307,62 @@ func TestWrapGRPCPublic(t *testing.T) { require.NotContains(t, errStr, "secret123") require.NotContains(t, errStr, "admin") }) + + t.Run("preserves error code through WrapGRPCPublic and UnwrapGRPC round-trip", func(t *testing.T) { + // This test verifies that clients (e.g. tx-blaster) can detect specific error + // types after WrapGRPCPublic sanitization via errors.Is() code matching. + tests := []struct { + name string + err *Error + sentinel *Error + }{ + {"TX_LOCKED", NewTxLockedError("transaction is locked"), ErrTxLocked}, + {"TX_EXISTS", NewTxExistsError("tx already exists"), ErrTxExists}, + {"TX_INVALID", NewTxInvalidError("invalid transaction"), ErrTxInvalid}, + {"SERVICE_ERROR", NewServiceError("service unavailable"), ErrServiceError}, + {"NOT_FOUND", New(ERR_NOT_FOUND, "resource not found"), ErrNotFound}, + {"PROCESSING", NewProcessingError("processing failed"), ErrProcessing}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Simulate server-side: wrap with WrapGRPCPublic + grpcErr := WrapGRPCPublic(tc.err) + require.NotNil(t, grpcErr) + + // Simulate client-side: unwrap with UnwrapGRPC + unwrapped := UnwrapGRPC(grpcErr) + require.NotNil(t, unwrapped) + + // The client should be able to detect the error type via errors.Is + require.True(t, unwrapped.Is(tc.sentinel), + "errors.Is should match %s after WrapGRPCPublic round-trip, got code %s", + tc.sentinel.code.String(), unwrapped.code.String()) + }) + } + }) + + t.Run("does not expose wrapped error chain through round-trip", func(t *testing.T) { + // A nested error chain should not survive the sanitization + deepErr := New(ERR_STORAGE_ERROR, "secret database connection string") + midErr := New(ERR_SERVICE_ERROR, "internal path /var/secrets", deepErr) + topErr := New(ERR_PROCESSING, "processing failed", midErr) + + grpcErr := WrapGRPCPublic(topErr) + unwrapped := UnwrapGRPC(grpcErr) + + // Should have the top-level code and message + require.Equal(t, ERR_PROCESSING, unwrapped.code) + require.Equal(t, "processing failed", unwrapped.message) + + // Should not have any wrapped error chain + require.Nil(t, unwrapped.wrappedErr) + + // Should not leak internal details in the error string + errStr := unwrapped.Error() + require.NotContains(t, errStr, "secret database") + require.NotContains(t, errStr, "/var/secrets") + }) } // TestWrapPublic tests the WrapPublic function for proper TError creation without internal details. From fc1b2c5b073ad9677e062aba257d052589b64290 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 11 Mar 2026 15:40:49 +0000 Subject: [PATCH 2/4] fix: address review - safe WithDetails handling, use package-level Is in tests --- errors/errors.go | 8 ++++++-- errors/errors_test.go | 14 +++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index 57174c6b2f..fb70d2611d 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -741,13 +741,17 @@ func WrapGRPCPublic(err error) error { st := status.New(ErrorCodeToGRPCCode(publicErr.code), publicErr.message) - // Attach a sanitized TError detail so clients can recover the application error code + // Attach a sanitized TError detail so clients can recover the application error code. + // This cannot practically fail since TError is a simple proto message, but if it does, + // we fall through and return the bare status (same as pre-fix behavior). detail, pbErr := anypb.New(&TError{ Code: publicErr.code, Message: publicErr.message, }) if pbErr == nil { - st, _ = st.WithDetails(detail) + if newSt, detailsErr := st.WithDetails(detail); detailsErr == nil { + st = newSt + } } return st.Err() diff --git a/errors/errors_test.go b/errors/errors_test.go index 3ff4e64f0b..b1388f5d89 100644 --- a/errors/errors_test.go +++ b/errors/errors_test.go @@ -2330,13 +2330,17 @@ func TestWrapGRPCPublic(t *testing.T) { grpcErr := WrapGRPCPublic(tc.err) require.NotNil(t, grpcErr) - // Simulate client-side: unwrap with UnwrapGRPC + // Simulate client-side: the package-level Is() handles gRPC-wrapped errors + // by calling UnwrapGRPC internally before comparing error codes + require.True(t, Is(grpcErr, tc.sentinel), + "Is(grpcErr, sentinel) should match %s after WrapGRPCPublic, got %v", + tc.sentinel.code.String(), grpcErr) + + // Also verify direct UnwrapGRPC + Is works (what propagation.Client does) unwrapped := UnwrapGRPC(grpcErr) require.NotNil(t, unwrapped) - - // The client should be able to detect the error type via errors.Is - require.True(t, unwrapped.Is(tc.sentinel), - "errors.Is should match %s after WrapGRPCPublic round-trip, got code %s", + require.True(t, Is(unwrapped, tc.sentinel), + "Is(unwrapped, sentinel) should match %s after UnwrapGRPC, got code %s", tc.sentinel.code.String(), unwrapped.code.String()) }) } From 1000a5b68faca297769636b408846ac741bf35a8 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 11 Mar 2026 15:52:58 +0000 Subject: [PATCH 3/4] fix: sanitize message with RemoveInvalidUTF8 before gRPC/protobuf marshaling --- errors/errors.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index fb70d2611d..507fb7d55e 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -739,14 +739,17 @@ func WrapGRPCPublic(err error) error { return nil } - st := status.New(ErrorCodeToGRPCCode(publicErr.code), publicErr.message) + // Sanitize message for valid UTF-8 to prevent gRPC/protobuf marshaling failures + sanitizedMsg := RemoveInvalidUTF8(publicErr.message) + + st := status.New(ErrorCodeToGRPCCode(publicErr.code), sanitizedMsg) // Attach a sanitized TError detail so clients can recover the application error code. - // This cannot practically fail since TError is a simple proto message, but if it does, - // we fall through and return the bare status (same as pre-fix behavior). + // After UTF-8 sanitization this cannot practically fail, but if it does we fall + // through and return the bare status (same as pre-fix behavior). detail, pbErr := anypb.New(&TError{ Code: publicErr.code, - Message: publicErr.message, + Message: sanitizedMsg, }) if pbErr == nil { if newSt, detailsErr := st.WithDetails(detail); detailsErr == nil { From 40152fb0e55d41f225768d649c70da620684adf3 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 11 Mar 2026 16:01:47 +0000 Subject: [PATCH 4/4] fix: add nil guard in test, clarify doc comment on message preservation --- errors/errors.go | 5 +++-- errors/errors_test.go | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index 507fb7d55e..0d17ba5fb8 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -725,10 +725,11 @@ func PublicError(err error) *Error { } } -// WrapGRPCPublic wraps an error for gRPC without exposing internal details. +// WrapGRPCPublic wraps an error for gRPC without exposing internal structured details. // It includes a sanitized TError detail (code + message only) so that UnwrapGRPC // can reconstruct the correct application error code on the client side. -// No file paths, line numbers, function names, wrapped error chains, or error data are included. +// Structured metadata such as file paths, line numbers, function names, wrapped error +// chains, and error data is omitted; the error message itself is preserved (UTF-8 sanitized). func WrapGRPCPublic(err error) error { if err == nil { return nil diff --git a/errors/errors_test.go b/errors/errors_test.go index b1388f5d89..8a38308af6 100644 --- a/errors/errors_test.go +++ b/errors/errors_test.go @@ -2238,6 +2238,7 @@ func TestWrapGRPCPublic(t *testing.T) { // Verify the detail contains only code and message, no internal info unwrapped := UnwrapGRPC(result) + require.NotNil(t, unwrapped) require.Equal(t, ERR_SERVICE_ERROR, unwrapped.code) require.Equal(t, "service unavailable", unwrapped.message) require.Empty(t, unwrapped.file)