Skip to content

fix: WrapGRPCPublic preserves error code via sanitized TError detail#582

Merged
freemans13 merged 4 commits intobsv-blockchain:mainfrom
freemans13:fix/wrap-grpc-public-preserve-error-code
Mar 11, 2026
Merged

fix: WrapGRPCPublic preserves error code via sanitized TError detail#582
freemans13 merged 4 commits intobsv-blockchain:mainfrom
freemans13:fix/wrap-grpc-public-preserve-error-code

Conversation

@freemans13
Copy link
Copy Markdown
Collaborator

@freemans13 freemans13 commented Mar 11, 2026

Summary

  • WrapGRPCPublic was creating a bare gRPC status with no TError details, causing UnwrapGRPC on the client side to always return ERR_ERROR (9) instead of the original application error code (e.g., ERR_TX_EXISTS, ERR_TX_INVALID, ERR_TX_LOCKED)
  • This broke all errors.Is() detection in gRPC clients like tx-blaster, causing TX_EXISTS errors to be misclassified as service errors and re-queued — creating duplicate transactions
  • Now attaches a single sanitized TError detail containing only Code and Message — no file paths, line numbers, function names, wrapped error chains, or error data are included

Problem

The error sanitization in PR #413 introduced WrapGRPCPublic which strips internal details for security. However, it also stripped the TError status detail entirely:

Server: Error{code: ERR_TX_EXISTS (33)} → WrapGRPCPublic → status(Internal, "tx already exists") [no details]
Client: UnwrapGRPC → len(st.Details()) == 0 → Error{code: ERR_ERROR (9)}  ← code LOST
Client: errors.Is(err, ErrTxExists) → 9 != 33 → FALSE ❌

Fix

WrapGRPCPublic now packs a sanitized TError into the gRPC status details:

Server: Error{code: ERR_TX_EXISTS (33)} → WrapGRPCPublic → status(Internal, "tx already exists") [TError{Code:33, Message:"tx already exists"}]
Client: UnwrapGRPC → extracts TError detail → Error{code: ERR_TX_EXISTS (33)}
Client: errors.Is(err, ErrTxExists) → 33 == 33 → TRUE ✅

The error code enum is not sensitive information. The security concern addressed by #413 was about leaking file paths, stack traces, wrapped error chains, and internal debug data — all of which remain stripped.

Error handling safety

  • st.WithDetails error is handled — st is only replaced on success (if newSt, detailsErr := st.WithDetails(detail); detailsErr == nil { st = newSt })
  • anypb.New failure (cannot practically happen for a simple 2-field proto) falls through to bare status (same as pre-fix behavior), which is better than returning a generic marshal error that would also lose the code
  • Comment in code explains the fallback rationale

Tests

  • Updated existing test to verify sanitized detail is present (code + message only, no internal info)
  • Added round-trip test using package-level Is() (matches real client behavior): tests both Is(grpcErr, sentinel) (gRPC-wrapped path) and Is(UnwrapGRPC(grpcErr), sentinel) (propagation.Client path) for TX_LOCKED, TX_EXISTS, TX_INVALID, SERVICE_ERROR, NOT_FOUND, PROCESSING
  • Added test verifying wrapped error chains don't survive the sanitization round-trip

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.
@freemans13 freemans13 requested a review from Copilot March 11, 2026 15:32
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 11, 2026

🤖 Claude Code Review

Status: Complete

Current Review:
No issues found. This PR successfully addresses a critical bug where error codes were lost during gRPC serialization.

Key improvements verified:

  • UTF-8 sanitization applied to prevent protobuf marshaling failures (errors/errors.go:744)
  • Safe WithDetails error handling with proper fallback (errors/errors.go:756-758)
  • Comprehensive round-trip testing covering 6 error types (errors_test.go:2312-2348)
  • Clear documentation distinguishing structured metadata from message content (errors/errors.go:728-732)
  • Proper nil guards in tests to prevent panics (errors_test.go:2241)

History:

  • ✅ Fixed: All Copilot review feedback addressed in latest commits
    • UTF-8 sanitization applied before status and detail creation
    • Test safety improved with nil checks
    • Documentation clarified regarding metadata vs message content

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates gRPC public error wrapping so that client-side UnwrapGRPC can recover the original application error code even after sanitization, preventing gRPC clients from misclassifying specific transaction errors.

Changes:

  • Attach a sanitized TError detail (code + message only) in WrapGRPCPublic.
  • Update/add tests to validate presence of sanitized details and round-trip code preservation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
errors/errors.go WrapGRPCPublic now includes a sanitized TError detail in the gRPC status.
errors/errors_test.go Tests updated/added to assert sanitized detail presence and round-trip behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sonarqubecloud
Copy link
Copy Markdown

@freemans13 freemans13 merged commit 8e810a5 into bsv-blockchain:main Mar 11, 2026
11 checks passed
@freemans13 freemans13 removed the request for review from gokutheengineer March 11, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants