fix(status): prevent panic on invalid base64 in grpc-status-details-bin by sauravzg · Pull Request #2618 · grpc/grpc-rust

sauravzg · 2026-04-30T18:57:18Z

Instead of panicking with .expect() when base64 decoding of the grpc-status-details-bin header fails, fall back to setting the status code to Unknown and include the decoding error in the message. This is consistent with how Tonic handles failures to decode the grpc-message header.

Added a unit test details_invalid_base64 to verify this behavior.

** I am not entirely sure if this panic was an intentional choide or this change counts as a breaking change, but I'd assume that not panicking would be considered good behavior**

dfawley · 2026-05-05T16:01:02Z

+        let (code, message, details) = match (error_message, details_result) {
+            (Ok(msg), Ok(det)) => (code, msg, det),
+            (Err(e), _) => {
                let error_message = format!("Error deserializing status message header: {e}");


Note that Go doesn't consider it an error that changes the status code in this case. I'm not sure about Java/C++/the spec.

For now, I've decided to keep things consistent with how tonic handles it after some discussion with arjan on it i.e. "unknown error" which is what tonic does for incorrect error message.

I'll loop lucio in to the review and get his opinion here, keeping the comment open.

Ah okay I see I think its fine to keep the prev status code I don't think anyone is depending on this behavior and I think this is prob just an impl gap. So happy to have this align with what other grpc crates do.

So, it seems that this may not be consistent across languages.

Go : https://github.com/grpc/grpc-go/blob/1c63fa5f5492fb6cb4cabf9847999ce469505f49/internal/status/status.go#L54-L58 - Seems to just ignore the errors here

Java: https://github.com/grpc/grpc-java/blob/0248e6f75605ac5007fa71c2a62e4f325a61d32d/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java#L271-L278 , https://github.com/grpc/grpc-java/blob/0248e6f75605ac5007fa71c2a62e4f325a61d32d/protobuf/src/main/java/io/grpc/protobuf/StatusProto.java#L180 indicate that it just gets bubbled up to application as runtime exception.

C++ : This was more difficult to find so I asked gemini, which seems to think it ends up returning internal error.

Anyways, given the lack of consistency of error handling , personally lean towards keeping the same behavior as "grpc message" for tonic , but I don't mind changing it to something else. @dfawley LMK what you'd like here.

Sure, keeping the tonic behavior for now is fine, but we should figure out the "right" behavior, too. I think C++ (or gemini's version of c++) is most likely what we want, but I'm not sure.

Looking at the places in Go that you pointed to, I think we're talking about different things.

This error is about failing to percent-decode the grpc-message header, whereas you seem to be looking at the grpc-status-details-bin proto.

In Go we just do our best to percent-decode, ignoring any errors while emitting, e.g. %yq comes out verbatim if the header contains %yq, since it's a not valid percent-encoding. Or %%2F would emit %/. And we leave the error code the same. It looks like tonic will convert the error into Unknown and modify the strong to include the prefix of percent-decoding failing. The code in Go for this is here:

https://github.com/grpc/grpc-go/blob/bb023f846c01c094fede9bfe591ef5c5369e59ca/internal/transport/http2_client.go#L1515

https://github.com/grpc/grpc-go/blob/bb023f846c01c094fede9bfe591ef5c5369e59ca/internal/transport/http_util.go#L268-L299

https://github.com/grpc/grpc-go/blob/bb023f846c01c094fede9bfe591ef5c5369e59ca/internal/transport/http2_client.go#L1515

https://github.com/grpc/grpc-go/blob/bb023f846c01c094fede9bfe591ef5c5369e59ca/internal/transport/http_util.go#L268-L299

The code in go that you linked to talks about grpc-message field. For this PR I am mostly concerned with "if we don't crash on invalid b64 for status details, what's the behavior that we'd like" .

It looks like tonic will convert the error into Unknown and modify the strong to include the prefix of percent-decoding failing.

Doesn't tonic simply return the utf error from decode with a new message?

The code in go that you linked to talks about grpc-message field. For this PR I am mostly concerned with "if we don't crash on invalid b64 for status details, what's the behavior that we'd like" .

I get that, but this comment is on the part of the code where Tonic is changing the status code when the grpc-message header fails to parse. I believe that behavior is incorrect. It should keep the status code the same instead of converting it to UNKNOWN. I'll try to confirm with the other languages today.

Instead of panicking with `.expect()` when base64 decoding of the `grpc-status-details-bin` header fails, fall back to setting the status code to `Unknown` and include the decoding error in the message. This is consistent with how Tonic handles failures to decode the `grpc-message` header. Added a unit test `details_invalid_base64` to verify this behavior.

LucioFranco · 2026-05-15T18:21:48Z

-                (Code::Unknown, error_message)
+                (Code::Unknown, error_message, Bytes::new())
+            }
+            (Ok(_msg), Err(e)) => {


do we want to do anything with the message instead of throwing it away?

So, that's probably a behavior we should define. Unfortunately, that's not consistently defined in grpc as one of the other comment discusses.

This approach treats invalid status details as a header and returns a completely different error unrelated to the RPC error. (This might be similar to what gemini thinks c++ behavior is)

The other approach would be to do something similar to go, where this is treated as a silent failure and we simply return original code and message.

I wouldn't want to change the error code but reuse the original error message.

Ultimately I don't think we want to be doing anything with grpc-status-details-bin at this low of a level. That requires a protobuf dependency to parse it. The generated code should deal with this trailer somehow instead.

https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#appendix-a---grpc-for-protobuf

(This clause only requires the use of a google.rpc.Status proto message in this header when you are using protobuf with gRPC. gRPC can transmit messages of any type.)

I don't think there's a protobuf dep here in tonic, tonic simply returns the serialized bin.

I don't think there's a protobuf dep here in tonic, tonic simply returns the serialized bin.

OK that's good.. So this is just a base64 decoding error on the grpc-status-details-bin header. I don't think we want to treat this any differently than decoding any other -bin header, ideally. In Go at least, any -bin header decoding error leads to Internal + "malformed header : ":

https://github.com/grpc/grpc-go/blob/609310837bbc7fab1553fa53f2d1312bd7d85275/internal/transport/http2_client.go#L1522-L1527
and
https://github.com/grpc/grpc-go/blob/609310837bbc7fab1553fa53f2d1312bd7d85275/internal/transport/http2_client.go#L1582-L1585

I guess in Tonic the other decoding happens only when the application accesses it from the metadata? But this one happens specially in the transport? If that's true there's probably not much more we can do here. I think the behavior for this header roughly matches Go's behavior (though we call it INTERNAL -- https://github.com/grpc/grpc/blob/master/doc/statuscodes.md doesn't specify -- "could not decompress" is INTERNAL, which seems closest?). But I don't think this exact status code is too important for now considering it isn't well defined. So I'd be fine with leaving it alone.

sauravzg requested review from arjan-bal and dfawley April 30, 2026 18:57

sauravzg force-pushed the sauravz/status-panic branch from 67250d1 to cd49dc4 Compare April 30, 2026 18:59

arjan-bal reviewed May 4, 2026

View reviewed changes

Comment thread tonic/src/status.rs

sauravzg force-pushed the sauravz/status-panic branch from cd49dc4 to 956a170 Compare May 4, 2026 07:13

sauravzg requested a review from arjan-bal May 4, 2026 07:15

dfawley reviewed May 5, 2026

View reviewed changes

sauravzg force-pushed the sauravz/status-panic branch from 956a170 to afeeea3 Compare May 7, 2026 16:22

sauravzg requested a review from LucioFranco May 7, 2026 16:25

sauravzg force-pushed the sauravz/status-panic branch from afeeea3 to 80d0094 Compare May 14, 2026 18:07

dfawley approved these changes May 14, 2026

View reviewed changes

LucioFranco reviewed May 15, 2026

View reviewed changes

dfawley assigned sauravzg May 21, 2026

Conversation

sauravzg commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sauravzg commented Apr 30, 2026 •

edited

Loading