fix(client): validate response content-type - #2831
Conversation
|
|
cd50591 to
8bb9a2d
Compare
|
Thanks for putting this together! Before we can merge, there are a few edge cases and potential breakages we should address: • Trailers-Only Responses: When a gRPC server returns an immediate error via a Trailers-Only response (where grpc-status is delivered directly in the headers), the server often omits content-type entirely since there is no body payload. Because this check currently runs before Status::from_header_map, it overwrites the real status code and message with a generic invalid content-type: error. We should ensure Status::from_header_map runs first; if a grpc-status header exists, we should respect it and skip the content-type check. If we adjust the ordering so grpc-status header inspection takes priority, that should eliminate the risk of clobbering real errors while still catching proxy misconfigurations. All that said, because we are winding down Tonic development even if we merge this there is no guarantee we'll have another release that will have it. Given that, let me know if you'd like to proceed! |
This checks that the response Content-Type begins with application/grpc and returns an error immediately if it does not, avoiding confusing decoding errors. Fixes grpc#2365
Trailers-Only responses deliver grpc-status directly in the initial headers and often omit the content-type header because there is no body payload. Prioritizing Status::from_header_map ensures real status codes and error messages are preserved and not overwritten with generic invalid content-type errors. Content-type validation now only runs when no grpc-status header is present.
4493613 to
1aa5fdc
Compare
|
Thanks @nathanielford for pointing that out, that makes total sense! I just pushed an update to address this. The ordering is flipped now so Also added an integration test for Trailers-Only responses without a And totally get it regarding Tonic winding down! I'd still love to get this merged into |
Fixes #2365.
Currently, if the gRPC client receives a response with an incorrect
Content-Type(for instance,text/htmlfrom a proxy like Envoy returning a502 Bad Gateway), it ignores the content type and proceeds to attempt decoding the HTML body as if it were a valid gRPC binary stream. This leads to confusing stream parsing errors, such asinvalid compression flag: 60 ... while receiving response with status: 502 Bad Gateway.According to the gRPC specification: "If the Content-Type does not begin with
application/grpc, the client SHOULD NOT assume that the body contains gRPC messages, and MUST fail the RPC with a status of UNKNOWN (or the equivalent status code translated from the HTTP status code, if one is present)."Solution
This PR adds a
Content-Typevalidation step intonic/src/client/grpc.rsduring the initialcreate_responsecall.We now check if the
Content-Typeheader starts withapplication/grpc(which also properly accommodatesapplication/grpc-webandapplication/grpc+proto). If it is missing or invalid, we immediately return an error. If there is a recognizable HTTP error code (e.g.,502), we utilizecrate::status::infer_grpc_statusto convert it to the appropriate gRPC status, but with a clear, custom error message stating that theContent-Typewas invalid.